[GISel] Don't fold convergent instruction across CFG

[qcolombet]

Summary

Before merging two instructions together, GISel does some sanity checks that the folding is legal.
However that check was missing that the source of the pattern may be convergent. When the destination location is in a different basic block, the folding is invalid.

[qcolombet]

Hi @arsenm ,

Do you know if AMDGPU has any convergent instruction involved in pattern with folding?
I could use that to create a test case.

Cheers,
-Quentin

[arsenm]

I'm not sure what context this gets called in. Maybe it's possible to hit this for DPP instructions?

llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp

63

The block based check will probably break down with convergence tokens. Could this just skip convergent altogether?

[qcolombet]

I'm not sure what context this gets called in. Maybe it's possible to hit this for DPP instructions?

This is called during instruction selection from the patterns that have been generated from SDISel.
Essentially, if you have a Pat that takes two or more instructions and fold them.

Thanks for the pointer, I'll take a look.

llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp

63

We could, but I was worried this would introduce regressions when the pattern would legitimately apply.
In particular, if we don't fold when the things are in the same block, we will regress compared to SDISel.

When the convergence tokens land (they are not landed right?), we'll have to fix SDISel, so maybe it is okay to have to fix GISel at that point?
I.e., for now let's match SDISel?

What do you think?

[arsenm]

In D128539#3616207, @qcolombet wrote:

I'm not sure what context this gets called in. Maybe it's possible to hit this for DPP instructions?

This is called during instruction selection from the patterns that have been generated from SDISel.
Essentially, if you have a Pat that takes two or more instructions and fold them.

Thanks for the pointer, I'll take a look.

I looked and don't see anything for this. We fold all of these in separate MI passes

llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp

63

OK, I guess this can match. Basically every isConvergent check will probably be broken anyway

[qcolombet]

I looked and don't see anything for this. We fold all of these in separate MI passes

Thanks for checking @arsenm !

That means I can't produce an open source test case unfortunately then.

Are you okay with the patch as is?

llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp

 
 bool InstructionSelector::isObviouslySafeToFold(MachineInstr &MI,
                                                 MachineInstr &IntoMI) const {
   // Immediate neighbours are already folded.
   if (MI.getParent() == IntoMI.getParent() &&
       std::next(MI.getIterator()) == IntoMI.getIterator())
     return true;
 
+  // Convergent instructions cannot be moved in the CFG.
+  if (MI.isConvergent() && MI.getParent() != IntoMI.getParent())
+    return false;
+
   return !MI.mayLoadOrStore() && !MI.mayRaiseFPException() &&
          !MI.hasUnmodeledSideEffects() && MI.implicit_operands().empty();
 }