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

[Vedant Kumar via cfe-commits]

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


> 
>>>> +  }
>>>> +
>>>>  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.

vedant

>  Maybe it doesn't matter either way, since Tool isn't really used for anything else...
> 
>> 
>>> 
>>>>  }
>>>>  return Result;
>>>> 
>>> 
>> 
>