[PATCH] D86294: [RFC] AMDGPU/GlobalISel: Look through copies in GIM_CheckOpcode and add post-isel hook

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:4538
+
+  if (isVOP3(MI.getOpcode())) {
+    // Make sure constant bus requirements are respected.
----------------
arsenm wrote:
> foad wrote:
> > arsenm wrote:
> > > arsenm wrote:
> > > > I was trying to avoid doing things this way. This is selecting instructions incorrectly, and then relying on something else to fix them up. I think we should select trivially correct instructions, and a follow on pass could do a better job figuring out the optimal SGPR operands. This is why RegBankSelect is forcing everything to VGPRs for VALU instructions. The later pass will have an easier time if everything is expected to consistently be in VGPRs, rather than having to figure out if some operands are already scalar and moving them back to VGPRs if it decided another copy was better
> > > We're also incorrectly letting some constant bus violations through with ThreeOp_i32_Pats, which do need a complex predicate to avoid the constant bus violations. Technically the later folding pass could form these cases, but it would be nice to keep these in patterns. I haven't thought of a good solution for this yet
> > How do we "select trivially correct instructions" if regbankselect has already run, and it has no foreknowledge of what complex patterns we might want to match as part of instruction selection? Some of the current failures to select v_lshl_add are because the inputs to the G_SHL are uniformn (or constant) and regbankselect doesn't know that we might want to combine it into a G_ADD whose other operand is divergent.
> It's trivial if you look at a single instruction at a time. The constant bus violations only start showing up when you start looking through copies when pattern matching multiple input operations.
> 
> The patterns have to make sure they don't violate the constant bus restriction. As I mentioned, ThreeOp_i32_Pats is broken and really should fail to import. ThreeOpFrag needs a non-trivial predicate that uses PredicateCodeUsesOperands, but GlobalISelEmitter doesn't implement it (or even check if it's in use). 
> It's trivial if you look at a single instruction at a time. The constant bus violations only start showing up when you start looking through copies when pattern matching multiple input operations.
> 
> The patterns have to make sure they don't violate the constant bus restriction. As I mentioned, ThreeOp_i32_Pats is broken and really should fail to import. ThreeOpFrag needs a non-trivial predicate that uses PredicateCodeUsesOperands, but GlobalISelEmitter doesn't implement it (or even check if it's in use). 




================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:4538
+
+  if (isVOP3(MI.getOpcode())) {
+    // Make sure constant bus requirements are respected.
----------------
foad wrote:
> arsenm wrote:
> > foad wrote:
> > > arsenm wrote:
> > > > arsenm wrote:
> > > > > I was trying to avoid doing things this way. This is selecting instructions incorrectly, and then relying on something else to fix them up. I think we should select trivially correct instructions, and a follow on pass could do a better job figuring out the optimal SGPR operands. This is why RegBankSelect is forcing everything to VGPRs for VALU instructions. The later pass will have an easier time if everything is expected to consistently be in VGPRs, rather than having to figure out if some operands are already scalar and moving them back to VGPRs if it decided another copy was better
> > > > We're also incorrectly letting some constant bus violations through with ThreeOp_i32_Pats, which do need a complex predicate to avoid the constant bus violations. Technically the later folding pass could form these cases, but it would be nice to keep these in patterns. I haven't thought of a good solution for this yet
> > > How do we "select trivially correct instructions" if regbankselect has already run, and it has no foreknowledge of what complex patterns we might want to match as part of instruction selection? Some of the current failures to select v_lshl_add are because the inputs to the G_SHL are uniformn (or constant) and regbankselect doesn't know that we might want to combine it into a G_ADD whose other operand is divergent.
> > It's trivial if you look at a single instruction at a time. The constant bus violations only start showing up when you start looking through copies when pattern matching multiple input operations.
> > 
> > The patterns have to make sure they don't violate the constant bus restriction. As I mentioned, ThreeOp_i32_Pats is broken and really should fail to import. ThreeOpFrag needs a non-trivial predicate that uses PredicateCodeUsesOperands, but GlobalISelEmitter doesn't implement it (or even check if it's in use). 
> > It's trivial if you look at a single instruction at a time. The constant bus violations only start showing up when you start looking through copies when pattern matching multiple input operations.
> > 
> > The patterns have to make sure they don't violate the constant bus restriction. As I mentioned, ThreeOp_i32_Pats is broken and really should fail to import. ThreeOpFrag needs a non-trivial predicate that uses PredicateCodeUsesOperands, but GlobalISelEmitter doesn't implement it (or even check if it's in use). 
> 
> 
OK, makes sense. We can look at implementing PredicateCodeUsesOperands for GlobalISel in tablegen.


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

https://reviews.llvm.org/D86294