This is an archive of the discontinued LLVM Phabricator instance.

Let llc and opt override "-target-cpu" and "-target-features" using command line options "-mcpu" and "-mattr"
ClosedPublic

Authored by ahatanak on May 6 2015, 1:13 PM.

Details

Summary

llc's options -mcpu and -mattr have no effect if the functions in the IR already have function attributes -target-cpu and -target-features. This is bad for llvm developers as they don't have a convenient way to quickly test different target-cpu and target-features via command line options.

This patch attempts to fix this bug by letting llc and opt rewrite the attributes in the IR. An alternate way would be to pass the cpu and feature strings in TargetOptions but I chose the approach I took in the patch to avoid making changes to TargetOptions and XXXTargetMachine. Also, factored out helper function getCPUFeaturesStr which both opt and llc call.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 25080.May 6 2015, 1:13 PM
ahatanak retitled this revision from to Let llc and opt override "-target-cpu" and "-target-features" using command line options "-mcpu" and "-mattr".
ahatanak updated this object.
ahatanak edited the test plan for this revision. (Show Details)
ahatanak added reviewers: echristo, dexonsmith.
ahatanak added a subscriber: Unknown Object (MLST).
echristo accepted this revision.May 6 2015, 1:26 PM
echristo edited edge metadata.

I agree, no TargetOptions for this. I don't necessarily agree with rewriting, but it seems to be a small enough change and will work for what the overrides are meant to be for. Some discipline in reviewing testcases to make sure that a lot of overrides aren't there for incoming testcases is going to be necessary for clean tests. One inline comment which basically sums up to "why not two functions"? As long as you have a reasonable answer go ahead.

Thanks!

-eric

include/llvm/CodeGen/CommandFlags.h
268

Why a pair?

This revision is now accepted and ready to land.May 6 2015, 1:26 PM

I agree, no TargetOptions for this. I don't necessarily agree with rewriting, but it seems to be a small enough change and will work for what the overrides are meant to be for. Some discipline in reviewing testcases to make sure that a lot of overrides aren't there for incoming testcases is going to be necessary for clean tests. One inline comment which basically sums up to "why not two functions"? As long as you have a reasonable answer go ahead.

I'm not sure if I understood your comment about testcases. Do you mean people shouldn't duplicate functions in test cases just to test different target-cpu and target-feature strings but instead should use -mcpu and -mattrs (which I think makes sense)? Should the one inline comment go into the test cases I added?

include/llvm/CodeGen/CommandFlags.h
268

Is it better to split this into two functions getCPUStr and getFeaturesStr? Or you meant the two strings (cpu and features strings) should be passed by reference?

This revision was automatically updated to reflect the committed changes.