[RegisterCoalescer] fix dst subreg replacement during remat copy trick

[ivafanas]

Summary

Instructions might use definition register as its "undef" operand. It happens on architectures with predicated executon:

%0:subreg = instruction op_1, ..., op_N, undef %0:subreg, op_N+2, ...

RegisterCoalescer should take into account all remat instruction operands during destination subregister fixup.

; remat result before fix:
%1 = instruction op_1, ..., op_N, undef %1:subreg, op_N+2, ...

; remat result after fix (correct):
%1 = instruction op_1, ..., op_N, undef %1, op_N+2, ...

[ivafanas]

Typo fix in comment

[ivafanas]

ping

[ivafanas]

Could anybody have a look, please?

[qcolombet]

Hi @ivafanas,

Any chance you can provide a test case with the patch?
In particular, I'd like to confirm how we end up in this situation. (I suppose we have a copy that fed back to a phi?)

For the record, I believe this problem has been here for ages, because trivial rematerialization has been limited to instruction with no operands for the most part. Put differently, I'm not surprised you're running into bugs like this.
I'm actually surprised your instruction is trivially rematerializable to begin with.

Cheers,
-Quentin

[ivafanas]

In D125657#3801453, @qcolombet wrote:

Hi @ivafanas,

Any chance you can provide a test case with the patch?
In particular, I'd like to confirm how we end up in this situation. (I suppose we have a copy that fed back to a phi?)

For the record, I believe this problem has been here for ages, because trivial rematerialization has been limited to instruction with no operands for the most part. Put differently, I'm not surprised you're running into bugs like this.
I'm actually surprised your instruction is trivially rematerializable to begin with.

Cheers,
-Quentin

Hi @qcolombet ,

We have triggered the issue on the custom out-of-tree architecture with predicated execution. That's why I can not provide a test case, unfortunately.
There is a common scenario for instruction to have the same register defined and "undef" used as operand.

Something like:

%2 = ADD %0, %1, undef %2, $sprf0, 0

Or

%1 = MOV 123, undef %1, $sprf0, 0

I've done my best to explain the problem in patch description and comment :) Don't know how to explain it better)

I'm not familiar with other architectures, but having in mind that the code exists for years and nobody triggered the issue, seems like it is not the case for other architectures.
Maybe it is ok that patch will live only in our fork, I'm not sure.

[qcolombet]

I'm not familiar with other architectures, but having in mind that the code exists for years and nobody triggered the issue, seems like it is not the case for other architectures.

Agree. In particular, other architectures likely won't have any argument on the rematerialized instructions.

[MatzeB]

When needing to test regalloc situations I sometimes write tests by creating a .mir file and just adding implicit-def and implicit-use operands on a "NOOP" instructions as necessary to re-create the particular situation. See here for a random example: https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/simple-register-allocation-read-undef.mir

When you just run a single pass that nothing will complain about the extra operands added...

[qcolombet]

When needing to test regalloc situations I sometimes write tests by creating a .mir file and just adding implicit-def and implicit-use operands on a "NOOP" instructions as necessary to re-create the particular situation.

Agree, although I don't think it will work in that case, since we need to trigger a rematerialization and that rematerialization needs to have operands and unless I'm mistaken the rematerialization won't carrying around the implicit operands.

[MatzeB]

In D125657#3806193, @qcolombet wrote:

When needing to test regalloc situations I sometimes write tests by creating a .mir file and just adding implicit-def and implicit-use operands on a "NOOP" instructions as necessary to re-create the particular situation.

Agree, although I don't think it will work in that case, since we need to trigger a rematerialization and that rematerialization needs to have operands and unless I'm mistaken the rematerialization won't carrying around the implicit operands.

Ah, yeah I don't know of an easy way to test with rematerialization either.

[ivafanas]

Hi @qcolombet , @MatzeB

I do not have commit access to LLVM repo.
If you think that LLVM should benefit from this patch, can I please ask you to apply changes.

If we are in doubt, let's abandon it and forget.

[qcolombet]

Hi @ivafanas,

I can do the commit on your behalf.
Which name and email should I use for that?
For the name I have "Afanasyev Ivan", but I have a hard time finding an email :P.

Cheers,
-Quentin

[ivafanas]

In D125657#3812162, @qcolombet wrote:

Hi @ivafanas,

I can do the commit on your behalf.
Which name and email should I use for that?
For the name I have "Afanasyev Ivan", but I have a hard time finding an email :P.

Cheers,
-Quentin

Afanasyev Ivan
ivafanas @ gmail.com

Thank you a lot!

llvm/lib/CodeGen/RegisterCoalescer.cpp

if (DstIdx != 0) {
     if (DefMO.getSubReg() == DstIdx) {
       assert(SrcIdx == 0 && CP.isFlipped()
              && "Shouldn't have SrcIdx+DstIdx at this point");
       const TargetRegisterClass *DstRC = MRI->getRegClass(DstReg);
       const TargetRegisterClass *CommonRC =
         TRI->getCommonSubClass(DefRC, DstRC);
       if (CommonRC != nullptr) {
         NewRC = CommonRC;
+
+        // Instruction might contain "undef %0:subreg" as use operand:
+        //   %0:subreg = instr op_1, ..., op_N, undef %0:subreg, op_N+2, ...
+        //
+        // Need to check all operands.
+        for (MachineOperand &MO : NewMI.operands()) {
+          if (MO.isReg() && MO.getReg() == DstReg && MO.getSubReg() == DstIdx) {
+            MO.setSubReg(0);
+          }
+        }
+
         DstIdx = 0;
-        DefMO.setSubReg(0);
         DefMO.setIsUndef(false); // Only subregs can have def+undef.
       }
     }
   }
 
   // CopyMI may have implicit operands, save them so that we can transfer them
   // over to the newly materialized instruction after CopyMI is removed.
   SmallVector<MachineOperand, 4> ImplicitOps;