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

[Vedant Kumar via cfe-commits]

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

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.