[PATCH] D44046: [AMDGPU] Fixed incorrect break from loop

[Tim Renouf via Phabricator via llvm-commits]

tpr added a comment.

The arguments of SI_IF_BREAK appear to be the "I want to break this time" mask and the running "already exited loop" mask. Therefore we need to ensure that the first one of those is masked with exec before it is ORed into the second one. See the test case for an example of where it is needed.



================
Comment at: lib/Target/AMDGPU/SILowerControlFlow.cpp:354
+  if (MI.getOperand(1).isReg())
+    if (MachineInstr *Def = MRI->getUniqueVRegDef(MI.getOperand(1).getReg()))
+      if (Def->getParent() == MI.getParent())
----------------
rampitec wrote:
> arsenm wrote:
> > Can you add a testcase where this will fail if there is no defining instruction (e.g. the source value was a function argument)
> Can this be a partial def?
I don't think it can be a partial def, as we know that it is something that was an i1 in the IR.


================
Comment at: lib/Target/AMDGPU/SILowerControlFlow.cpp:354
+  if (MI.getOperand(1).isReg())
+    if (MachineInstr *Def = MRI->getUniqueVRegDef(MI.getOperand(1).getReg()))
+      if (Def->getParent() == MI.getParent())
----------------
tpr wrote:
> rampitec wrote:
> > arsenm wrote:
> > > Can you add a testcase where this will fail if there is no defining instruction (e.g. the source value was a function argument)
> > Can this be a partial def?
> I don't think it can be a partial def, as we know that it is something that was an i1 in the IR.
I'll give the suggested test a try, but I suspect it won't like me passing an i1 as an arg.


================
Comment at: lib/Target/AMDGPU/SILowerControlFlow.cpp:356
+      if (Def->getParent() == MI.getParent())
+        SkipAnding = SIInstrInfo::isVALU(*Def);
+
----------------
arsenm wrote:
> This doesn't seem like the right condition to me. This should probably be a predicate that checks the specific operand for being a result that is masked with exec? Theoretically this could be something exotic like a write lane.
> 
> This also could be = parentmatch && isFoo() rather than an if
The operand is the break condition, which is something that was an i1 in the IR, but is now a pair of sgprs (including the case of vcc). If it's a valu, I don't think that can be anything other than an instruction with carry-out, i.e. v_cmp (or possibly the carry-out from v_add_u). I should probably add a comment to this effect.

As an optimization, it could maybe also spot other cases, principally an s_and_b64 with exec that's already there. But I don't think that is needed for this commit.


Repository:
  rL LLVM

https://reviews.llvm.org/D44046