> On 2016-Jul-13, at 11:18, Vedant Kumar <v...@apple.com> wrote: > >> >> On Jul 13, 2016, at 10:30 AM, Duncan P. N. Exon Smith <dexonsm...@apple.com> >> wrote: >> >> >>> 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? > > 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.
Okay, this seems like a great cleanup then. LGTM. > > 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. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits