[PATCH] D133012: [AMDGPU] Add subtarget feature for MAD_U64/I64 bug on GFX11

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/VOP3Instructions.td:300
+    defm V_MAD_U64_U32_strict : VOP3Inst <"v_mad_u64_u32", VOP3b_I64_I1_I32_I32_I64>;
+    defm V_MAD_I64_I32_strict : VOP3Inst <"v_mad_i64_i32", VOP3b_I64_I1_I32_I32_I64>;
+  }
----------------
mbrkusanin wrote:
> Joe_Nash wrote:
> > mbrkusanin wrote:
> > > Joe_Nash wrote:
> > > > For other types of instructions (VOPC, VOP2) opName is used as a key into mapping tables, so having duplicate entries with the same name can cause collisions. As long as there are no functional issues, I'm fine with it as is. It will be a tablegen failure if someone tries to add a mapping table and use that as the key in the future.
> > > Is current solutions preferable to having two versions for Real instruction? Problem with having two Reals is we would need to disable one for decoding because of conflict and then manually adjust it, which does not look nice. Encoding is easily resolved with just some extra predicates.
> > I don't quite understand what you are asking. Currently we have 2 pseudos and one real for each subtarget. We should not need multiple reals on a single subtarget. The way to resolve my comment fully is to go to the following. Which is NFC currently.
> > 
> > defm V_MAD_U64_U32_strict : VOP3Inst <"v_mad_u64_u32**_strict**", VOP3b_I64_I1_I32_I32_I64>;
> > defm V_MAD_I64_I32_strict : VOP3Inst <"v_mad_i64_i32**_strict**", VOP3b_I64_I1_I32_I32_I64>;
> Actually it would not be NFC. Mapping non-strict pseudo to real would fail for: llc -mcpu=gfx1100 -mattr=-mad-intra-fwd-bug.
> The point of the patch is to have an option to disable the workaround for the bug.
> 
> Maybe I misunderstood what the first comment meant. Patch as-is matches both Pseudos into same Real. Is the potential issue for a new mapping table in the future if that table wants to use opName as key? Or are you talking about mapping Reals back into Pseudos?
> Would ,,let PseudoInstr = " for pseudo make a difference (opName would be different then)?
> Actually it would not be NFC. Mapping non-strict pseudo to real would fail for: llc -mcpu=gfx1100 -mattr=-mad-intra-fwd-bug.
> The point of the patch is to have an option to disable the workaround for the bug.
> 
Ah, I missed this part where you wanted to turn off the feature on gfx11. Perhaps there should be a test for that.

> Maybe I misunderstood what the first comment meant. Patch as-is matches both Pseudos into same Real. Is the potential issue for a new mapping table in the future if that table wants to use opName as key? Or are you talking about mapping Reals back into Pseudos?
> Would ,,let PseudoInstr = " for pseudo make a difference (opName would be different then)?

"a new mapping table in the future if that table wants to use opName as key" is the part I was worried about. It's OK to leave it for now and address it later if it becomes a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133012