> On 2016-Jul-12, at 18:59, Vedant Kumar <v...@apple.com> wrote: > >> >> On Jul 12, 2016, at 6:23 PM, Duncan P. N. Exon Smith <dexonsm...@apple.com> >> wrote: >> >> >>> On 2016-Jul-12, at 17:47, Vedant Kumar <v...@apple.com> wrote: >>> >>> >>>> On Jul 12, 2016, at 5:41 PM, Duncan P. N. Exon Smith >>>> <dexonsm...@apple.com> wrote: >>>> >>>> >>>>> On 2016-Jul-12, at 16:53, Vedant Kumar <v...@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. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits