RegAllocFast: Do not free later early-clobbered registers.

[FreddyYe]

Summary

Consider corner case: MSInlineAsm may contain operands of both
implicit-def $eax and implicit-def early-clobber $eax

[FreddyYe]

This patch is to fix a regression bug (https://gcc.godbolt.org/z/GG44f6) on recent rewrite on RegAllocFast.cpp. The situation that implicit-def $eax and implicit-def early-clobber $eax both occur in MI is rare. But since old implement can handle it, so I add a some condition to check such MI.

[LuoYuanke]

llvm/test/CodeGen/X86/phys-msInline-fastregalloc.ll

11

It seems to be invalid inline assembly. We can't set eax both as output and clobber. The clobbered register and output register should be in different physical register.

[llvm]$ cat t.c

void foo(int a, int b) {
  int c;
  asm volatile("addl %1, %2 \n"
               "movl %2, %0\n"
               :"=a"(c)
               :"r"(a), "r"(b)
               :"eax");
}
[llvm]$ clang -S t.c
t.c:8:17: error: asm-specifier for input or output variable conflicts with asm clobber list
               :"eax");
                ^
1 error generated.
[llvm]$ gcc -S t.c
t.c: In function ‘foo’:
t.c:4:3: error: ‘asm’ operand has impossible constraints
    4 |   asm volatile("addl %1, %2 \n"
      |   ^~~
[llvm]$

This is from https://llvm.org/docs/LangRef.html#output-constraints.

If this is not safe (e.g. if the assembly contains two instructions, where the first writes to one output, and the second reads an input and writes to a second output), then the “&” modifier must be used (e.g. “=&r”) to specify that the output is an “early-clobber” output. Marking an output as “early-clobber” ensures that LLVM will not use the same register for any inputs (other than an input tied to this output).

The eax is an output, so front-end need to mark the early-clobber for eax with "=&{eax}", so it seems to be a front-end issue?

[FreddyYe]

llvm/test/CodeGen/X86/phys-msInline-fastregalloc.ll

11

Thanks for review! Sorry for failing to know this output constraint in IR before. Yes, both GNU inline asm and llvm IR don't allow setting one register both as output and clobber. So the front-end seems to deal wrong with MS inline assembler. Seems like clang front-end always generate ={eax} and set each output register in asm as clobber when dealing with MS inline assembler. So this conflict may happen in clang front-end. I will abandon this review.

[FreddyYe]

llvm/test/CodeGen/X86/phys-msInline-fastregalloc.ll

11

I'll double check on conclusions above.

[arsenm]

I'd like an additional MIR test for this

[FreddyYe]

Add MIR test

[arsenm]

llvm/lib/CodeGen/RegAllocFast.cpp

1241

I don't understand what's special about MSVC inline asm. Aren't the constraints semantically the same for any asm type?

llvm/test/CodeGen/X86/corner-msInline-fastregalloc.mir

5–24 ↗

(On Diff #314589)

You should be able to drop the whole IR section

26–62 ↗

(On Diff #314589)

Can delete most of this (-simplify-mir helps)

[FreddyYe]

llvm/lib/CodeGen/RegAllocFast.cpp

1241

MSVC inline asm is wrote in Intel assembly format. So the method isMsInlineAsm() here uses flag inteldialect to distinguish from GNU inline asm. (I don't know why this flag didn't show in this MIR test, but it indeed works.)

From the investigation I've done so far, I have several conclusions:

1. MSVC inline asm doesn't specify semantically input, output, clobber in source. But LLVM IR has such semantics. So does GNU inline asm.
2. When clang front-end deals with MS inline asm, it has such rules to set input, output, clobber:
   1. always set eax as output
   2. set the dest register in each asm as a clobber

The rules above will then generate corner case like this patch's IR and MIR test.

Yuanke mentioned about set early clobber in LLVM IR (i.e. =&r). I did a experiment. It indeed can pass this bug without this patch. But seems like this rule is for user side. Becasue I found that clang also cannot set =&r for GNU inline asm when output register is written to before the last asm. But clang can deal with similar semantics GNU inline case, because eax is not set as clobber by front-end. (https://gcc.godbolt.org/z/We3b9c).

[FreddyYe]

simplify the MIR test

[FreddyYe]

THX for review!

llvm/test/CodeGen/X86/corner-msInline-fastregalloc.mir

5–24 ↗

(On Diff #314589)

I failed to drop the IR. Because if dropped, %stack.0.retval and %stack.1.c will parsed by llc as not defined. Maybe I'm doing the wrong way. I'm not so familiar with MIR. Can you show me an example also drops IR but with stack variable?

[FreddyYe]

I uploaded another patch in front-end D94466. It also can fix this regression bug. Both solutions are reasonable to me, since for now clang is weak to deal with MS inline asm.

[arsenm]

llvm/lib/CodeGen/RegAllocFast.cpp

1241

There shouldn't be a semantic difference. Ultimately the MIR should have the same flags/semantics regardless of the syntax used. This is either correct for all asm dialects, or it's generally incorrect

[FreddyYe]

llvm/lib/CodeGen/RegAllocFast.cpp

1241

I agree with you that there shouldn't be semantic difference. Maybe the old implementation just happen to cover this issue luckily. I'd like to listen to more front-end's voice for this issue. Welcome to comment there! D94466

[FreddyYe]

Patch has solved this regression bug in Front End.

[FreddyYe]

Patch has solved this regression bug in Front End.

llvm/lib/CodeGen/RegAllocFast.cpp

 //===- RegAllocFast.cpp - A fast register allocator for debug code --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
for (unsigned I = MI.getNumOperands(); I-- > 0;) {
       }
 
       // Do not free tied operands and early clobbers.
       if (MO.isTied() || MO.isEarlyClobber())
         continue;
       Register Reg = MO.getReg();
       if (!Reg)
         continue;
+      // Do not free later early-clobbered registers when MI is MSVCInlineAsm
+      if (MI.isMSInlineAsm()) {
+        unsigned J = 0;
+        for (J = I+1; J != MI.getNumOperands(); ++J) {
+          MachineOperand &LaterMO = MI.getOperand(J);
+          if (LaterMO.isReg() && LaterMO.getReg() == Reg &&
+              MI.getOperand(J).isEarlyClobber())
+            break;
+        }
+        if (J != MI.getNumOperands())
+          continue;
+      }
       assert(Reg.isPhysical());
       if (MRI->isReserved(Reg))
         continue;
       freePhysReg(Reg);
       unmarkRegUsedInInstr(Reg);
     }
   }
 

llvm/test/CodeGen/X86/phys-msInline-fastregalloc.ll

+; RUN: llc < %s -O0 -mtriple=i386-unknown-linux-gnu -mcpu=generic -regalloc=fast -optimize-regalloc=0 -no-x86-call-frame-opt | FileCheck %s
+
+define dso_local i32 @main() {
+entry:
+; CHECK: movl $1, %eax
+; CHECK-NOT: movl %eax, (%eax)
+  %retval = alloca i32, align 4
+  %c = alloca { [4 x i32] }, align 4
+  store i32 0, i32* %retval, align 4
+  %m32 = bitcast { [4 x i32] }* %c to [4 x i32]*
+  %0 = call i32 asm sideeffect inteldialect "mov eax,$$1\0A\09mov $0,eax", "=*m,={eax},~{eax},~{dirflag},~{fpsr},~{flags}"([4 x i32]* %m32) #1, !srcloc !3
+  store i32 %0, i32* %retval, align 4
+  ret i32 0
+}
+!3 = !{i32 285}
 No newline at end of file