This is an archive of the discontinued LLVM Phabricator instance.

Change clang driver to pass -target-abi option
ClosedPublic

Authored by vmedic on Nov 3 2014, 8:04 AM.

Diff Detail

Event Timeline

vmedic updated this revision to Diff 15711.Nov 3 2014, 8:04 AM
vmedic retitled this revision from to Change clang driver to pass -target-abi option.
vmedic updated this object.
vmedic edited the test plan for this revision. (Show Details)
vmedic added a reviewer: dsanders.
vmedic changed the visibility from "Public (No Login Required)" to "Custom Policy".
dsanders changed the visibility from "Custom Policy" to "Public (No Login Required)".Nov 3 2014, 8:57 AM
dsanders added a subscriber: atanasyan.

It needs a testcase and I don't see where Opts.ABI is used.

By the way, please don't set custom access policies. There's no need for them.

tools/driver/cc1as_main.cpp
82

Not initialized in constructor.

Also, can you point me at the code that uses this variable? I can only see the code that sets it.

atanasyan edited edge metadata.Nov 3 2014, 10:17 AM

Not sure that the getTargetFeatures routine is a good place to configure the -target-abi option.

lib/Driver/Tools.cpp
1793

The getTargetFeatures routine called from the ClangAs::ConstructJob and from the Clang::ConstructJob. In the second case we add the -target-abi option twice. The first time when we call the getTargetFeatures and the second time when we call Clang::AddMIPSTargetArgs. I do not think it is a good idea.

vmedic added a comment.Nov 6 2014, 7:42 AM
In D6091#8, @atanasyan wrote:

Not sure that the getTargetFeatures routine is a good place to configure the -target-abi option.

I was suspicious about this myself, but couldn't find the proper place to put the code in. Do you please have any idea where it would suite better?

Maybe create new method ClangAs::AddMIPSTargetArgs with -target-abi handling and call this method from ClangAs::ConstructJob. I think that code duplication is a minor problem here.

vmedic updated this revision to Diff 16140.Nov 13 2014, 1:40 AM
vmedic edited edge metadata.

A new method ClangAs::AddMIPSTargetArgs with -target-abi handling has been created. This method is called from ClangAs::ConstructJob to handle this particular task.

atanasyan added inline comments.Nov 13 2014, 5:17 AM
tools/driver/cc1as_main.cpp
192

Where do we use the value stored in the Opts.ABI?

vmedic added inline comments.Nov 13 2014, 6:38 AM
tools/driver/cc1as_main.cpp
192

I I understood correctly these options are passed to assembler when it is invoked with -cc1 option. Please correct me if I'm wrong.

And the patch still needs a test case.

tools/driver/cc1as_main.cpp
192

At the line cc1as_main.cpp:82 you add new field ABI to the AssemblerInvocation structure. At the line cc1as_main.cpp:191 you initialize this filed by the -target-abi arguments. But where is this new field used? For example the AssemblerInvocation::CPU filed is initialized at the line 186 and passed to the createMCSubtargetInfo at the line 357.

vmedic updated this revision to Diff 16290.Nov 17 2014, 8:07 AM

Removed ABI field from AssemblerInvocation class. Added test cases for integrated assembler launch.

atanasyan accepted this revision.Nov 17 2014, 8:29 AM
atanasyan edited edge metadata.

LGTM with the only notes

lib/Driver/Tools.cpp
1

Unnecessary change.

This revision is now accepted and ready to land.Nov 17 2014, 8:29 AM
echristo added a subscriber: echristo.

You'll want to remove the -target-feature bits of this code... honestly it shouldn't even compile at this point.

Actually this patch to the testcase doesn't appear to be against top of tree - can you rebase against that?

Thanks.

-eric

vmedic planned changes to this revision.Jan 30 2015, 7:43 AM
vmedic updated this revision to Diff 19036.Jan 30 2015, 7:46 AM
vmedic edited edge metadata.

Patch is rebased to the TOT. Passing -target-feature o32,n32,n64 options is disabled to remove warnings.

This revision is now accepted and ready to land.Jan 30 2015, 7:46 AM
vmedic requested a review of this revision.Jan 30 2015, 7:55 AM
vmedic edited edge metadata.

Please review the updated patch.

vmedic edited edge metadata.Jan 30 2015, 7:56 AM
vmedic added a subscriber: Unknown Object (MLST).
dsanders accepted this revision.Jan 30 2015, 8:08 AM
dsanders edited edge metadata.

LGTM. The passing of -target-abi ought to be generic given that the backend handling is generic but this is still good as-is since it will silence the unknown feature warnings coming from the backend and fixing that is fairly urgent.

lib/Driver/Tools.cpp
4868

Nit: Indentation

4985

Nit: Indentation

This revision is now accepted and ready to land.Jan 30 2015, 8:08 AM
dsanders closed this revision.Jan 30 2015, 9:36 AM