This is an archive of the discontinued LLVM Phabricator instance.

[RFC, 4.0 branch] Enable FeatureFlatForGlobal on Volcanic Islands
ClosedPublic

Authored by rivanvx on Jan 17 2017, 8:38 AM.

Details

Summary

Add FeatureFlatForGlobal to Volcanic Islands features and update the tests to contain -mattr=-flat-for-global.

This patch is intended for 4.0 branch to avoid assert failure on Volcanic Islands due to missing ADDR64 instructions. There is a better approach targeted for trunk based on arsenm's patches: https://github.com/arsenm/llvm/tree/movetovalu-addr64 which I will post at a later date.

Diff Detail

Event Timeline

rivanvx created this revision.Jan 17 2017, 8:38 AM
rivanvx edited the summary of this revision. (Show Details)Jan 17 2017, 8:39 AM
rivanvx updated this revision to Diff 84905.Jan 18 2017, 4:16 PM
rivanvx edited the summary of this revision. (Show Details)

Better approach. I have hardcode the processor names in AMDGPUSubtarget::initializeSubtargetDependencies() because getGeneration() will return SI before ParseSubtargetFeatures() finishes, and I have to provide Feature String for it to execute.

Still have two Failing Tests (2):

  • LLVM :: CodeGen/AMDGPU/fcanonicalize.f16.ll
  • LLVM :: CodeGen/AMDGPU/fdiv.ll

Not sure why these failures occur. Suggestions welcome.

arsenm added inline comments.Jan 19 2017, 10:35 AM
lib/Target/AMDGPU/AMDGPUSubtarget.cpp
45–46

The flat-for-global should be added once under the combined condition.

Also I think the kernel does enable unaligned accesses on the supported hardware, so we should revisit just enabling this always

47–51

You should check the generation, not hardcode a bunch of GPU device names

rivanvx added inline comments.Jan 20 2017, 5:00 AM
lib/Target/AMDGPU/AMDGPUSubtarget.cpp
45–46

I will check.

47–51

That was my initial idea as well, but it can only be done after ParseSubtargetFeatures() is finished, which needs FS. So we have to hardcode the GPU names.

rivanvx updated this revision to Diff 85168.Jan 20 2017, 11:46 AM
rivanvx edited the summary of this revision. (Show Details)

Addressed all the issues mentioned. Please review.

arsenm added inline comments.Jan 20 2017, 12:31 PM
lib/Target/AMDGPU/AMDGPUSubtarget.cpp
47–51

You could also directly set the FlatForGlobal member without going through the feature string

rivanvx updated this revision to Diff 85296.Jan 22 2017, 12:15 PM

Updated per @arsenm's comment.

arsenm accepted this revision.Jan 23 2017, 11:46 AM

LGTM assuming all tests pass now

This revision is now accepted and ready to land.Jan 23 2017, 11:46 AM
arsenm closed this revision.Jan 24 2017, 2:16 PM

r292982. I found a better way than checking the string contents and pruned some of the test changes