Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The encoding is a bit different from what we use downstream, because I don't like the idea of assigning the is-high flag to the LSB. This makes encodings that we get from getEncodingValue() for the non-16bit registers effectively incorrect and thus requiring additional treatment. Placing the flag in one of the MSBs feels like it extends the existing encoding conventions more naturally.
The hi/lo bit was specifically chosen to be the LSB in the Register encoding so that subtracting registers creates a logical register range. This is used for True16 codegen support SIInsertWaitcnts, and likely elsewhere.
I do not think it is a good idea to introduce changes while upstreaming the feature because they cannot be tested against the codegen implementation downstream. There is not enough context for code inspection or test coverage upstream to support whether that bit layout change is a good idea.
The hi/lo bit was specifically chosen to be the LSB in the Register encoding so that subtracting registers creates a logical register range. This is used for True16 codegen support SIInsertWaitcnts, and likely elsewhere.
Why do SIInsertWaitcnts intervals need to represent individual 16-bit registers?
After seeing the changes to SIInsertWaitcnts and testing this downstream, I think it will work fine.
LGTM
llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp | ||
---|---|---|
506 | For these initial patches I would like to keep changes minimal and avoid any immediate reworks and refinements. We can always do that later (it's in my TODO list). | |
llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_vop2.txt | ||
70 | Well, it just exposes the current level of support for these instructions, and gives them some coverage. I believe we didn't break anything here. |
Why not to call TRI->getHWRegIndex(AMDGPU::getMCReg(Op.getReg(), *ST)) here and remove any dependency on the encoding?