[PATCH] Move --march, --mcpu, and --mattr to lli.cpp

Both llc.cpp and JIT/TargetSelect.cpp defined these flags, meaning that when I link all of LLVM's libraries into a single shared library, llc crashes on startup with duplicate flag definitions.

This patch moves the flag definitions into lli.cpp and passes them through the EngineBuilder into JIT::selectTarget().

[Jeffrey Yasskin, 2010-02-03 03:03:28]

Please take a look. The current patch is at
http://codereview.appspot.com/download/issue199072_1.diff

[baldrick_free.fr, 2010-02-03 07:33:25]

Hi,

> Please review this at http://codereview.appspot.com/199072/show

what is the advantage of using this codereview thingy?

Ciao,

Duncan.

[Jeffrey Yasskin (google), 2010-02-03 16:18:17]

On Tue, Feb 2, 2010 at 11:33 PM, Duncan Sands <baldrick@free.fr> wrote:
> Hi,
>
>> Please review this at http://codereview.appspot.com/199072/show
>
> what is the advantage of using this codereview thingy?

You can double-click on a line you want to comment on, and all the
context is already there. And when I update the patchset, everyone can
immediately see that I've changed it and what I've changed, rather
than having to re-scan the diff.

But Rietveld (codereview.appspot.com) puts the "Please review" line
into the email on its own. I don't really care if you want to review
the diff instead (which I linked and is available through the
"Download raw patch set" link on Rietveld).

Jeffrey

[clattner_apple.com, 2010-02-04 07:25:56]

On Feb 2, 2010, at 7:03 PM, jyasskin@gmail.com wrote:

> Reviewers: llvm-commits_cs.uiuc.edu,
> 
> Message:
> Please take a look. The current patch is at
> http://codereview.appspot.com/download/issue199072_1.diff
> 
> Description:
> Both llc.cpp and JIT/TargetSelect.cpp defined these flags, meaning that
> when I link all of LLVM's libraries into a single shared library, llc
> crashes on startup with duplicate flag definitions.
> 
> This patch moves the flag definitions into TargetMachine (which made
> sense in my head, but the exact location doesn't really matter).

I like the spirit of this patch, but it will cause tools (e.g. clang, which
doesn't use the jit but does use targets) to get new options that they
shouldn't.  Would it be possible to hoist the JIT command line options out into
lli.cpp?

-Chris

[Jeffrey Yasskin (google), 2010-02-04 23:31:30]

On Wed, Feb 3, 2010 at 11:25 PM, Chris Lattner <clattner@apple.com> wrote:
>
> On Feb 2, 2010, at 7:03 PM, jyasskin@gmail.com wrote:
>
>> Reviewers: llvm-commits_cs.uiuc.edu,
>>
>> Message:
>> Please take a look. The current patch is at
>> http://codereview.appspot.com/download/issue199072_1.diff
>>
>> Description:
>> Both llc.cpp and JIT/TargetSelect.cpp defined these flags, meaning that
>> when I link all of LLVM's libraries into a single shared library, llc
>> crashes on startup with duplicate flag definitions.
>>
>> This patch moves the flag definitions into TargetMachine (which made
>> sense in my head, but the exact location doesn't really matter).
>
> I like the spirit of this patch, but it will cause tools (e.g. clang, which
doesn't use the jit but does use targets) to get new options that they
shouldn't.  Would it be possible to hoist the JIT command line options out into
lli.cpp?
>

Sure. Done at http://codereview.appspot.com/download/issue199072_2002.diff
or http://codereview.appspot.com/199072.

[daniel_zuster.org, 2010-02-05 03:32:41]

On Thu, Feb 4, 2010 at 3:31 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
> On Wed, Feb 3, 2010 at 11:25 PM, Chris Lattner <clattner@apple.com> wrote:
>>
>> On Feb 2, 2010, at 7:03 PM, jyasskin@gmail.com wrote:
>>
>>> Reviewers: llvm-commits_cs.uiuc.edu,
>>>
>>> Message:
>>> Please take a look. The current patch is at
>>> http://codereview.appspot.com/download/issue199072_1.diff
>>>
>>> Description:
>>> Both llc.cpp and JIT/TargetSelect.cpp defined these flags, meaning that
>>> when I link all of LLVM's libraries into a single shared library, llc
>>> crashes on startup with duplicate flag definitions.
>>>
>>> This patch moves the flag definitions into TargetMachine (which made
>>> sense in my head, but the exact location doesn't really matter).
>>
>> I like the spirit of this patch, but it will cause tools (e.g. clang, which
doesn't use the jit but does use targets) to get new options that they
shouldn't.  Would it be possible to hoist the JIT command line options out into
lli.cpp?
>>
>
> Sure. Done at http://codereview.appspot.com/download/issue199072_2002.diff
> or http://codereview.appspot.com/199072.

Much better, we definitely do not want to increase the visibility of
the backend's command line options.

Would it make sense to have optional arguments for these -- most
simple clients aren't going to care to pass them, and it keeps source
compatibility.

 - Daniel

> _______________________________________________
> llvm-commits mailing list
> llvm-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>

[Jeffrey Yasskin (google), 2010-02-05 04:29:28]

On Thu, Feb 4, 2010 at 7:32 PM, Daniel Dunbar <daniel@zuster.org> wrote:
> On Thu, Feb 4, 2010 at 3:31 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>> Sure. Done at http://codereview.appspot.com/download/issue199072_2002.diff
>> or http://codereview.appspot.com/199072.
>
> Much better, we definitely do not want to increase the visibility of
> the backend's command line options.
>
> Would it make sense to have optional arguments for these -- most
> simple clients aren't going to care to pass them, and it keeps source
> compatibility.

That's what the EngineBuilder is for. Instead of making people
remember the order of 10 different parameters, and pass the defaults
for earlier ones if they want to set a later one, they create an
EngineBuilder, set exactly the parameters they want, and then call
create().

Jeffrey

[daniel_zuster.org, 2010-02-05 07:05:11]

On Thu, Feb 4, 2010 at 8:29 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
> On Thu, Feb 4, 2010 at 7:32 PM, Daniel Dunbar <daniel@zuster.org> wrote:
>> On Thu, Feb 4, 2010 at 3:31 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>>> Sure. Done at http://codereview.appspot.com/download/issue199072_2002.diff
>>> or http://codereview.appspot.com/199072.
>>
>> Much better, we definitely do not want to increase the visibility of
>> the backend's command line options.
>>
>> Would it make sense to have optional arguments for these -- most
>> simple clients aren't going to care to pass them, and it keeps source
>> compatibility.
>
> That's what the EngineBuilder is for. Instead of making people
> remember the order of 10 different parameters, and pass the defaults
> for earlier ones if they want to set a later one, they create an
> EngineBuilder, set exactly the parameters they want, and then call
> create().

Ok.

 - Daniel

>
> Jeffrey
>

[Jeffrey Yasskin (google), 2010-02-05 16:23:04]

On Thu, Feb 4, 2010 at 11:05 PM, Daniel Dunbar <daniel@zuster.org> wrote:
> On Thu, Feb 4, 2010 at 8:29 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>> On Thu, Feb 4, 2010 at 7:32 PM, Daniel Dunbar <daniel@zuster.org> wrote:
>>> On Thu, Feb 4, 2010 at 3:31 PM, Jeffrey Yasskin <jyasskin@google.com> wrote:
>>>> Sure. Done at http://codereview.appspot.com/download/issue199072_2002.diff
>>>> or http://codereview.appspot.com/199072.
>>>
>>> Much better, we definitely do not want to increase the visibility of
>>> the backend's command line options.
>>>
>>> Would it make sense to have optional arguments for these -- most
>>> simple clients aren't going to care to pass them, and it keeps source
>>> compatibility.
>>
>> That's what the EngineBuilder is for. Instead of making people
>> remember the order of 10 different parameters, and pass the defaults
>> for earlier ones if they want to set a later one, they create an
>> EngineBuilder, set exactly the parameters they want, and then call
>> create().
>
> Ok.

Thanks! Committed as r95390.