This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Implement infrastructure to set options in AMDGPUToolChain
ClosedPublic

Authored by kasaurov on Sep 1 2017, 10:40 AM.

Details

Summary

In current OpenCL implementation some options are set in OpenCL RT/Driver, which causes discrepancy between online and offline paths.
Implement infrastructure to move options from OpenCL RT/Driver to AMDGPUToolChain using overloaded TranslateArgs() method.
Create map for default options values, as Options.td doesn't support default values (in contrast with OPTIONS.def).
Add two driver options: -On and -mNN (like -O3, -m64).
Some minor formatting changes to follow the clang-format style.

Diff Detail

Repository
rL LLVM

Event Timeline

kasaurov created this revision.Sep 1 2017, 10:40 AM
emankov added inline comments.Sep 1 2017, 10:50 AM
lib/Driver/ToolChains/AMDGPU.cpp
60–62 ↗(On Diff #113549)

redundant braces

74–75 ↗(On Diff #113549)

One hasArg might be used for all.

lib/Driver/ToolChains/AMDGPU.h
50 ↗(On Diff #113549)

const Ref might be returned here.

51–52 ↗(On Diff #113549)

Check on not found opt is needed.

60 ↗(On Diff #113549)

Return arg on the same line.

rampitec added inline comments.Sep 1 2017, 11:11 AM
lib/Driver/ToolChains/AMDGPU.h
44 ↗(On Diff #113549)

Is it really needed to create map in the header?

yaxunl edited edge metadata.Sep 1 2017, 5:00 PM

Is it possible to add a test for this? Thanks.

t-tye added a subscriber: msearles.Sep 1 2017, 5:36 PM
kasaurov updated this revision to Diff 113767.Sep 4 2017, 9:26 AM

Several requested changes:

  • move map initialization out of header file -- visually it looks better in .h file, but logically it should be in .cpp
  • add assert() in getOptionDefault() -- check for unknown to OptionsDefault option
  • combine args for OPT_O check
  • use const StrinRef instead of std::string in the map and getOptionDefault() -- that eliminates conversion in AddJoinedArg()
  • eliminate redundant brackets

ToDo:

  • test
kasaurov added inline comments.Sep 4 2017, 9:33 AM
lib/Driver/ToolChains/AMDGPU.h
60 ↗(On Diff #113549)

That's how clang-format sees it:
-DerivedArgList *AMDGPUToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch, Action::OffloadKind DeviceOffloadKind) const {
+DerivedArgList *
+AMDGPUToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch,
+ Action::OffloadKind DeviceOffloadKind) const {

This revision is now accepted and ready to land.Sep 4 2017, 10:15 AM
kasaurov updated this revision to Diff 113780.Sep 4 2017, 11:02 AM

Test added to check passing of -On/default

yaxunl accepted this revision.Sep 4 2017, 4:14 PM

LGTM. Thanks.

emankov accepted this revision.Sep 5 2017, 2:33 AM

LGTM

This revision was automatically updated to reflect the committed changes.