This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Use enum for flat variants. NFC
ClosedPublic

Authored by sebastian-ne on Apr 1 2021, 9:30 AM.

Details

Summary

Use an enum FlatInstVariant to differentiate between the different
variants of flat instructions (flat, global and scratch).
This should make it easier to bundle the immediate offset logic in a
single place and implement restrictions and bug workarounds.

Fixed version of D99587, which does not rely on the address space.

Diff Detail

Event Timeline

sebastian-ne created this revision.Apr 1 2021, 9:30 AM
sebastian-ne requested review of this revision.Apr 1 2021, 9:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2021, 9:30 AM
foad added a comment.Apr 1 2021, 10:22 AM

Is there some overlap with SIInstrFlags::FLAT, SIInstrFlags::IsFlatScratch, SIInstrFlags::IsFlatGlobal? It seems like your enum is encoding the same thing in a slightly different way. See also the is*FLAT* helper methods in SIInstrInfo.

Use SIInstrFlags instead of new enum.

foad added a comment.Apr 8 2021, 1:09 AM

Use SIInstrFlags instead of new enum.

Thanks for trying it out! I think it is good to avoid introducing another enum for the same thing. I'm not thrilled about having to pass around a "uint64_t" argument for it, or about the name "SIInstrFlags::IsFlatScratch" which sounds more like a predicate function (but that's not your fault). What do others think?

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.h
16

I don't think you need this.

arsenm added a comment.Apr 8 2021, 5:54 AM

Can we just drop the Is from the bit name?

sebastian-ne marked an inline comment as done.

Rebased to drop Is from bit names and removed now unneeed include.

foad added a comment.Apr 9 2021, 2:44 AM

Incidentally I think it would be simpler if AMDGPU::getNumFlatOffsetBits did not take a "signed" argument, always returned the total number of bits in the field, and it was the caller's responsibility to worry about whether negative offsets are allowed or not.

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1668

Does this fix a real bug when AS == AMDGPUAS::GLOBAL_ADDRESS? If so it could do with a test case.

sebastian-ne added inline comments.Apr 9 2021, 2:49 AM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1668

I don’t think it is possible to create a flat load with a global address, at least everything I tried failed to compile. I’m happy to add a test case if someone manages to do that.

foad accepted this revision.Apr 9 2021, 2:55 AM
This revision is now accepted and ready to land.Apr 9 2021, 2:55 AM
This revision was landed with ongoing or failed builds.Apr 9 2021, 3:34 AM
This revision was automatically updated to reflect the committed changes.