[PATCH] D131254: [AMDGPU][GISel] Enable Selection of ADD3 for G_PTR_ADD

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/VOP3Instructions.td:605
+def : ThreeOp_Pats <add, add, V_ADD3_U32_e64>;
+def : ThreeOp_Pats <ptradd, ptradd, V_ADD3_U32_e64, [p3, i32, i32]>;
+def : ThreeOp_Pats <ptradd, ptradd, V_ADD3_U32_e64, [p5, i32, i32]>;
----------------
The globalisel pattern importer ignores the address space here. See the comment on `class PointerToAnyOperandMatcher`. So you only need one of these lines, and it will match any address space not just 3 or 5. And furthermore if you remove one of these lines then it stops GlobalISelEmitter from hoisting the predicate check call, so you don't need the FIXME above.


================
Comment at: llvm/lib/Target/AMDGPU/VOP3Instructions.td:459
+
+      // FIX: Somehow my changes made this predicate run before the
+      // Feature check, so it crashes without this.
----------------
Pierre-vh wrote:
> foad wrote:
> > Should be "FIXME". But it would be good to understand and fix this before committing.
> I'd also like to understand why too but so far I haven't found an answer. It seems like the GlobalISelEmitter is just emitting the feature check in the operand instruction matcher and not in the "parent" block.
> 
> Maybe @arsenm has an idea?
Right, it seems like a GlobalISelEmitter bug, like it is hoisting the predicate check call but does not realise that the predicate uses Operands[], so it would also need to hoist the GIM_RecordNamedOperands. There are some comments that might be relevant in `GroupMatcher::candidateConditionMatches` (about GIM_RecordInsn not GIM_RecordNamedOperand). But I don't really understand GlobalISelEmitter too well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131254