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

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?

>>> +  }
>>> +
>>>  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)?  Maybe it doesn't matter either way, since Tool isn't really used for anything else...

> 
>> 
>>>  }
>>>  return Result;
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160712/05d670a0/attachment-0001.html>