[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-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?
> 
> 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.