[PATCH] D22290: [PATCH 2/2] [Driver] Compute effective target triples once per job (NFCI)

[Duncan P. N. Exon Smith via cfe-commits]

> On 2016-Jul-13, at 11:18, Vedant Kumar <vsk at apple.com> wrote:
> 
>> 
>> On Jul 13, 2016, at 10:30 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> 
>>> On 2016-Jul-12, at 18:59, Vedant Kumar <vsk at apple.com> wrote:
>>> 
>>>> 
>>>> On Jul 12, 2016, at 6:23 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>> 
>>>> 
>>>>> On 2016-Jul-12, at 17:47, Vedant Kumar <vsk at apple.com> wrote:
>>>>> 
>>>>> 
>>>>>> On Jul 12, 2016, at 5:41 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 2016-Jul-12, at 16:53, Vedant Kumar <vsk at apple.com> wrote:
>>>>>>> 
>>>>>>> vsk created this revision.
>>>>>>> vsk added a reviewer: dexonsmith.
>>>>>>> vsk added a subscriber: cfe-commits.
>>>>>>> Herald added subscribers: srhines, danalbert, tberghammer, aemerson.
>>>>>>> 
>>>>>>> Compute an effective target triple exactly once in ConstructJob(), and
>>>>>>> then simply pass around const references to it. This eliminates wasteful
>>>>>>> re-computation of effective triples (e.g in getARMFloatABI()).
>>>>>>> 
>>>>>>> http://reviews.llvm.org/D22290
>>>>>>> 
>>>>>>> Files:
>>>>>>> include/clang/Driver/SanitizerArgs.h
>>>>>>> include/clang/Driver/Tool.h
>>>>>>> include/clang/Driver/ToolChain.h
>>>>>>> lib/Driver/Driver.cpp
>>>>>>> lib/Driver/SanitizerArgs.cpp
>>>>>>> lib/Driver/ToolChain.cpp
>>>>>>> lib/Driver/ToolChains.cpp
>>>>>>> lib/Driver/ToolChains.h
>>>>>>> lib/Driver/Tools.cpp
>>>>>>> lib/Driver/Tools.h
>>>>>>> 
>>>>>>> <D22290.63757.patch>
>>>>>> 
>>>>>>> Index: lib/Driver/Driver.cpp
>>>>>>> ===================================================================
>>>>>>> --- lib/Driver/Driver.cpp
>>>>>>> +++ lib/Driver/Driver.cpp
>>>>>>> @@ -2112,7 +2112,21 @@
>>>>>>>                                          AtTopLevel, MultipleArchs),
>>>>>>>                    BaseInput);
>>>>>>> 
>>>>>>> +  llvm::Triple EffectiveTriple;
>>>>>>> +  const ArgList &Args = C.getArgsForToolChain(TC, BoundArch);
>>>>>>> +  if (InputInfos.size() != 1) {
>>>>>>> +    EffectiveTriple = llvm::Triple(
>>>>>>> +        T->getToolChain().ComputeEffectiveClangTriple(Args));
>>>>>>> +  } else {
>>>>>>> +    // Pass along the input type if it can be unambiguously determined.
>>>>>>> +    EffectiveTriple =
>>>>>>> +        llvm::Triple(T->getToolChain().ComputeEffectiveClangTriple(
>>>>>>> +            Args, InputInfos[0].getType()));
>>>> 
>>>> It looks to me like this could call InputInfos[0].getType() and pass it into ComputeEffectiveClangTriple in cases where previously the type-less overload (which defaults to types::TY_Invalid) would have been called.
>>> 
>>> You're right that this is a behavior change!
>>> 
>>> The site you identified below (in ComputeLLVMTriple()) looks like the only
>>> place where ComputeEffectiveClangTriple's output is affected by InputType.
>>> 
>>>> 
>>>> Previously, the type was only specified in ClangAs::ConstructJob.  It only seems to make a difference for this logic in ToolChain::ComputeLLVMTriple:
>>>> 
>>>>  if ((InputType != types::TY_PP_Asm && Args.hasFlag(options::OPT_mthumb,
>>>>       options::OPT_mno_thumb, ThumbDefault)) || IsMProfile) {
>>>>    if (IsBigEndian)
>>>>      ArchName = "thumbeb";
>>>>    else
>>>>      ArchName = "thumb";
>>>>  }
>>>> 
>>>> Is it possible that a single input file will be types::TY_PP_ASM for a caller other than ClangAs?
>>>> - If not, why not?
>>>> - If so, this looks like a behaviour change.  Is that a bugfix, or a new bug?
> 
> SHAVE::Assembler is the only other tool which expects to deal with exactly one
> input with type TY_PP_Asm. However, not all implementations of ConstructJob()
> require or compute effective triples. In this case, the SHAVE assembler doesn't
> use effective triples, so the way we compute them in BuildJobsForActionNoCache()
> can't alter its behavior.
> 
> ISTM that the patch as-written doesn't change the effective triples seen by
> Tools which actually require effective triples. So, I think I spoke too soon
> when I said this is a behavior change!
> 
> To recap: We only pass InputInfos[0].getType() to ComputeEffectiveClangTriple()
> when there's a single InputInfo. This can only break implementations which (1)
> actually use effective triples and (2) have custom triple-creation code which
> *always* expects InputType to be TY_INVALID.
> 
> We don't have any implementations like that in tree.

Okay, this seems like a great cleanup then.  LGTM.

> 
> vedant
> 
>>> 
>>> I'm not sure. I'll try and come up with a test that exposes a behavior change.
>> 
>> Okay, great.
>> 
>>> 
>>>> 
>>>>>>> +  }
>>>>>>> +
>>>>>>> if (CCCPrintBindings && !CCGenDiagnostics) {
>>>>>>> +    // FIXME: We should be able to use the effective triple here, but doing so
>>>>>>> +    // breaks some multi-arch tests.
>>>>>> 
>>>>>> This is interesting.  Why does it break the tests?
>>>>> 
>>>>> It breaks these two tests:
>>>>> 
>>>>> test/Driver/darwin-dsymutil.c
>>>>> test/Driver/darwin-debug-version.c
>>>>> 
>>>>> The tests expect "-ccc-print-bindings" to spit out a default triple, namely:
>>>>> 
>>>>> x86_64-apple-darwin10
>>>>> 
>>>>> Printing out the effective triple means we get this instead:
>>>>> 
>>>>> x86_64-apple-macosx10.6.0
>>>>> 
>>>>> There's some chance that the test is too strict. I'm planning on keeping this
>>>>> patch NFCI and following up with the test authors later.
>>>> 
>>>> SGTM.
>>>> 
>>>>>> 
>>>>>>> llvm::errs() << "# \"" << T->getToolChain().getTripleString() << '"'
>>>>>>>              << " - \"" << T->getName() << "\", inputs: [";
>>>>>>> for (unsigned i = 0, e = InputInfos.size(); i != e; ++i) {
>>>>>>> @@ -2122,7 +2136,7 @@
>>>>>>> }
>>>>>>> llvm::errs() << "], output: " << Result.getAsString() << "\n";
>>>>>>> } else {
>>>>>>> -    T->ConstructJob(C, *JA, Result, InputInfos,
>>>>>>> +    T->ConstructJob(C, *JA, Result, InputInfos, EffectiveTriple,
>>>>>>>                 C.getArgsForToolChain(TC, BoundArch), LinkingOutput);
>>>>>> 
>>>>>> Why doesn't this have the same problem as above?  I.e., what happens for
>>>>>> multi-arch cases?
>>>>> 
>>>>> Methods which override ConstructJob used to call ComputeEffectiveClangTriple
>>>>> themselves, so they "expect" effective triples.
>>>> 
>>>> Rather than adding the effective triple to all the argument lists, does it make sense to call T->setEffectiveTriple() or T->computeEffectiveTriple(DefaultTriple)?
>>> 
>>> This gets a bit hairy because a Tool or ToolChain can outlive a single call to
>>> BuildJobsForActionNoCache(). That makes it hard to figure out exactly when a
>>> triple stored within one of these objects is invalidated.
>>> 
>>> It doesn't help that BuildJobsForActionNoCache() and BuildJobsForAction() are
>>> mutually recursive...
>>> 
>>> AFAICT effective triples should be passed around as arguments for the same
>>> reason the ArgList references are: they tend to change.
>> 
>> That makes sense.  Thanks for explaining.