[PATCH] D121229: [AMDGPU] Split FeatureAtomicFaddInsts feature. NFC.

Wed Mar 9 09:57:36 PST 2022

rampitec added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/FLATInstructions.td:1602

-let SubtargetPredicate = HasAtomicFaddInstsGFX90X in {
+let SubtargetPredicate = isGFX8GFX9NotGFX940 in {
+// These instructions are encoded differently on gfx90* and gfx940.
----------------
foad wrote:
> foad wrote:
> > rampitec wrote:
> > > GFX8?
> > Yeah, I agree this looks strange. I can clean it up if you like. I think we get away with it because these instructions also pick up OtherPredicates = [HasAtomicFaddXXXInsts] from their pseudos, and those features are never set on GFX8.
> I can replace this with isGFX908orGFX90A. I think it makes the patch cleaner, **but** it causes test/CodeGen/AMDGPU/global-atomics-fp-wrong-subtarget.ll to fail because it does this:
> ```
> ; Make sure we can encode and don't fail on functions which have
> ; instructions not actually supported by the subtarget.
> ```
> I have no idea why it does that, or whether this is still required to work for some reason, but at least the current version of this patch preserves this behaviour.
@arsenm I think that was your comment and test. Can you elaborate?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121229/new/

https://reviews.llvm.org/D121229