[MachineInstr] In addRegisterKilled and addRegisterDead, don't remove operands from inline assembly instructions if they have an associated flag operand.

[craig.topper]

Summary

INLINEASM instructions use extra operands to carry flags. If a register operand is removed without removing the flag operand, then the flags will no longer make sense.

This patch tries to fix this by preventing the removal when a flag operand is present. I have no idea if this is the right thing to do here.

This fixes a crash for the following test. Previously the Live Variable analysis would remove the edx and eax clobber operands since they are redundant with the rax and rdx operands. But it did not remove their flag operands. A later pass expected a register operand to follow the flag operand, but it was no longer there.

This code was created with MS inline assembly which added clobbers for each instruction without knowing about sub/super registers.

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@test_mem = dso_local global [16 x i8] c"UUUUUUUUUUUUUUUU", align 16

; Function Attrs: nounwind uwtable
define dso_local void @foo() local_unnamed_addr {
entry:
  tail call void asm sideeffect inteldialect "clc\0A\09cmpxchg8b $0\0A\09cmpxchg16b $1\0A\09clc", "=*m,=*m,~{eax},~{edx},~{flags},~{rax},~{rdx},~{dirflag},~{fpsr},~{flags}"([16 x i8]* nonnull @test_mem, [16 x i8]* nonnull @test_mem) #1
  ret void
}

[rnk]

Sounds good, it could use an IR test though.

BTW, if you want to clean up these clobber lists for MS inline asm, they are computed in llvm/lib/MC/MCParser/AsmParser.cpp. I think that has access to MCRegisterInfo, which knows the super/sub register relationships. This fix on its own seems like a good idea, though.

[craig.topper]

Add the test case from the description.

[rnk]

lgtm

[craig.topper]

I'm going to assume Reid meant to hit approve instead of changes required.

[rnk]

Right, sorry about that. :)

[malharJ]

The added condition

findInlineAsmFlagIdx(OpIdx) < 0))

avoids removing the Machine operand altogether (if there exists a corresponding inline asm flag),
was there any reason why the inline asm flag was not updated instead ?
ie. the code could still remove the machine operand but update (or remove it completely?) the asm flag to reflect that there is now one less operand ?

[efriedma]

I assume the reason is just that it would be complicated to rewrite the asm flags.

lib/CodeGen/MachineInstr.cpp

if (Reg == IncomingReg) {
       if (RegInfo->isSubRegister(IncomingReg, Reg))
         DeadOps.push_back(i);
     }
   }
 
   // Trim unneeded kill operands.
   while (!DeadOps.empty()) {
     unsigned OpIdx = DeadOps.back();
-    if (getOperand(OpIdx).isImplicit())
+    if (getOperand(OpIdx).isImplicit() &&
+        (!isInlineAsm() || findInlineAsmFlagIdx(OpIdx) < 0))
       RemoveOperand(OpIdx);
     else
       getOperand(OpIdx).setIsKill(false);
     DeadOps.pop_back();
   }
 
   // If not found, this means an alias of one of the operands is killed. Add a
   // new implicit operand if required.
if (MOReg == Reg) {
       if (RegInfo->isSubRegister(Reg, MOReg))
         DeadOps.push_back(i);
     }
   }
 
   // Trim unneeded dead operands.
   while (!DeadOps.empty()) {
     unsigned OpIdx = DeadOps.back();
-    if (getOperand(OpIdx).isImplicit())
+    if (getOperand(OpIdx).isImplicit() &&
+        (!isInlineAsm() || findInlineAsmFlagIdx(OpIdx) < 0))
       RemoveOperand(OpIdx);
     else
       getOperand(OpIdx).setIsDead(false);
     DeadOps.pop_back();
   }
 
   // If not found, this means an alias of one of the operands is dead. Add a
   // new implicit operand if required.

test/CodeGen/X86/ms-inline-asm-redundant-clobber.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+
+; This used to crash due to Live Variable analysis removing the redundant eax
+; and edx clobbers, but not removing the inline asm flag operands that proceed
+; them.
+
+@test_mem = dso_local global [16 x i8] c"UUUUUUUUUUUUUUUU", align 16
+
+; Function Attrs: nounwind uwtable
+define dso_local void @foo() local_unnamed_addr {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    #APP
+; CHECK:         clc
+; CHECK-NEXT:    cmpxchg8b {{.*}}(%rip)
+; CHECK-NEXT:    cmpxchg16b {{.*}}(%rip)
+; CHECK-NEXT:    clc
+; CHECK:         #NO_APP
+; CHECK-NEXT:    retq
+entry:
+  tail call void asm sideeffect inteldialect "clc\0A\09cmpxchg8b $0\0A\09cmpxchg16b $1\0A\09clc", "=*m,=*m,~{eax},~{edx},~{flags},~{rax},~{rdx},~{dirflag},~{fpsr},~{flags}"([16 x i8]* nonnull @test_mem, [16 x i8]* nonnull @test_mem) #1
+  ret void
+}