Details
- Reviewers
arsenm msearles rampitec t-tye - Group Reviewers
Restricted Project - Commits
- rG42bd81410e36: AMDGPU: Force sc0 and sc1 on stores for gfx940 and gfx941
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Should this be a feature set by default in the subtarget constructor instead? Should you be able to turn this off?
Users should not be able to turn this off. If user wants it off, user can use gfx942. Should it be moved to the subtarget constructor?
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp | ||
---|---|---|
524 | NIT: Is the use of the bitwise or " | " intended? I'd use the logical or " || " instead. |
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp | ||
---|---|---|
524 | It is intentional, we need both SC0 and SC1 bits set. If I switch this to || it will short circuit and not invoke enableSC1Bit. |
I think that if this is a new property of the GFX940/941 targets, and turning it off shouldn't be possible, we shouldn't even bother with a feature and just set a bool in the ST for those targets
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp | ||
---|---|---|
524 | IMHO then it needs a comment to explain that it's intentional, otherwise some innocent maintainer in the future could think it's a typo and change it |
AFAIK, the subtarget only knows the generation and has access to features. The subtarget cannot differentiate between gfx940, gfx941, gfx942.
Address review comments.
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp | ||
---|---|---|
524 | Added comment. |
llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp | ||
---|---|---|
517–529 | It may be a bit more verbose, but I would suggest just having a bool Changed variable, like other (admittedly more complicated) functions in the unit do. I don't think any future maintainer will mis-interpret this, even without the comment |
llvm/lib/TargetParser/TargetParser.cpp | ||
---|---|---|
330–332 ↗ | (On Diff #521322) | I don't see a reason to set this here. There's no need to expose this to the IR. We already leak too many target details here as it is |
NIT: Is the use of the bitwise or " | " intended? I'd use the logical or " || " instead.