Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
The most of the changes are for tests where the presence of the TFE operands doesn't make much sense and the code changes seem compact enough to not bother with making non-existing instructions possible and then slashing them with custom checks as we do for MIMGs. Also agrees with the existing practice of having the LDS and Offset/Index/Both variants as separate instructions.
For some reason SP3 additionally accepts TFE forms with vN VData operands where v[N:N+1] is expected, so for example buffer_load_dword v1, v0, s[4:7], s1 glc tfe and buffer_load_dword v[1:2], v0, s[4:7], s1 glc tfe are both accepted and produce same opcodes. It should be possible to support that with a separate change, if needed.
Apart from that, https://reviews.llvm.org/D19584 mentions that use of off operands agrees with SP3. I wasn't able to find any signs in SP3 behaviour confirming this.
AFAIK, tfe should be supported for loads and atomics only. I think that there is no sense to use tfe with stores.
SP3 is very forgiving and does not check many limitations.
For some reason SP3 additionally accepts TFE forms with vN VData operands where v[N:N+1] is expected, so for example buffer_load_dword v1, v0, s[4:7], s1 glc tfe and buffer_load_dword v[1:2], v0, s[4:7], s1 glc tfe are both accepted and produce same opcodes. It should be possible to support that with a separate change, if needed.
I think we do not have to support this SP3 feature.
Apart from that, https://reviews.llvm.org/D19584 mentions that use of off operands agrees with SP3. I wasn't able to find any signs in SP3 behaviour confirming this.
Unfortunately, SP3 is not consistent in accepting 'off' for unused operands. It does support off for FLAT operands, but not for MUBUF.
AFAIK, tfe should be supported for loads and atomics only. I think that there is no sense to use tfe with stores.
That was my guess as well. But isn't it right that we still want to be able to disassemble TFE stores, even if functionally useless?
That was my guess as well. But isn't it right that we still want to be able to disassemble TFE stores, even if functionally useless?
You are right, that would be useful. But I suggest correcting the parser to trigger an error if a store is used with tfe.
You are right, that would be useful. But I suggest correcting the parser to trigger an error if a store is used with tfe.
Do you mind if we do it with a separate follow-up patch? We already have some TFE stores in tests and whatever we do with them here might be masked by the amount of other test changes.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp | ||
---|---|---|
7704 | Remove IsLds now it is unused? |
llvm/lib/Target/AMDGPU/BUFInstructions.td | ||
---|---|---|
2286–2290 | Is there a reason to use both isTFE and noTFE? The expressions like !not(noTFE) are difficult to read. |
llvm/lib/Target/AMDGPU/BUFInstructions.td | ||
---|---|---|
2286–2290 | noTFE means we don't want the TFE version, for which isTFE is true, so not the same thing. !not(noTFE) indeed catches the eye, but I have no better options and it looks rather minor. Would appreciate any suggestions, though. |
llvm/lib/Target/AMDGPU/BUFInstructions.td | ||
---|---|---|
2286–2290 | Maybe hasTFE or enableTFE (with default=1) would be more readable? |
Remove IsLds now it is unused?