This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Register effective triple before get arm float abi.
ClosedPublic

Authored by aoli on Jul 21 2017, 3:40 PM.

Event Timeline

aoli created this revision.Jul 21 2017, 3:40 PM
aoli updated this revision to Diff 107998.Jul 24 2017, 5:14 PM

Move tests to a better place.

aoli added a reviewer: mgorny.Jul 24 2017, 5:15 PM

Kristof and Peter: Is this ok? Everything seems reasonable to me, but I wanted to make sure that you were aware of this, since it is ARM-related. Thanks.

peter.smith edited edge metadata.Jul 28 2017, 5:43 AM

The change looks like an improvement to me. I've got some comments/suggestions although I don't have any strong opinions here.

  • It isn't obvious from the context of Driver.cpp why ComputeEffectiveTriple() needs to be called. After a bit of searching I found the call to getArchNameForCompilerRTLib that contained the call to getARMFloatABI. Is is worth moving the call to ComputeEffectiveTriple just before the call to getARMFloatABI()?
  • It would be good to add some tests for some more targets such as --target=arm-linux-gnueabihf and --target=arm-linux-gnueabi -mfloat-abi=hard

Added compnerd and jroelofs to see if they have any comments or concerns.

aoli added a comment.Jul 28 2017, 10:02 AM
  • It isn't obvious from the context of Driver.cpp why ComputeEffectiveTriple() needs to be called. After a bit of searching I found the call to getArchNameForCompilerRTLib that contained the call to getARMFloatABI. Is is worth moving the call to ComputeEffectiveTriple just before the call to `()?

I was also thinking about that but it seems that ToolChain::getCompilerRT is also being called by many other functions. If we register effective triple right before the call to getARMFloatABI it will replace the origin EffectiveTriple in Toolchain. This may cause some side effects we don't want.

aoli updated this revision to Diff 108666.Jul 28 2017, 10:03 AM

Add more tests.

compnerd added inline comments.Jul 28 2017, 10:44 PM
lib/Driver/Driver.cpp
1311

Is there any reason to not hoist this irrespective of the runtime type? Basically, if we are going to be printing the "libgcc" filename, we compute and register the effective triple before we determine the location.

aoli added inline comments.Jul 29 2017, 12:17 PM
lib/Driver/Driver.cpp
1311

It is not required to register the effective triple before printing libgcc filename. We only need to do so for compiler-rt runtimes. Is there any benefit to hoist this irrespective of the runtime type?

compnerd edited edge metadata.Jul 30 2017, 11:40 AM

Right, I realize that it is only needed for the compiler-rt case, but I think it simplifies the flow and makes the behaviour similar across the two branches.

Thanks for adding the extra tests. It seems like getARMFloatABI used to compute its own EffectiveTriple but this was changed to retrieve a cached value D22596, this is a pity for options . With this in mind I think putting the RegisterEffectiveTriple within the scope of OPT_print_libgcc_file_name. I agree with compnerd that hoisting it out of the switch makes the flow of the code easier to read, but no strong opinions. No further comments from me.

aoli updated this revision to Diff 109681.Aug 3 2017, 9:57 PM

Hoist RegisterEffectiveTriple.

aoli marked an inline comment as done.Aug 22 2017, 4:27 PM

@peter.smith and @compnerd: do you think this patch is good to go now?

compnerd accepted this revision.Aug 22 2017, 10:07 PM
This revision is now accepted and ready to land.Aug 22 2017, 10:07 PM
This revision was automatically updated to reflect the committed changes.