[PATCH] D20456: [CodeGen] Fix problem with X86 byte registers in CriticalAntiDepBreaker

[Matthias Braun via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

This patch looks good enough to me now. I still have a question (see below) but that may be because of my limited understanding of the pass.

Thanks for the .mir test, it should already be more robust than the .ll one. I think there is room for simplifying it further though:


================
Comment at: lib/CodeGen/CriticalAntiDepBreaker.cpp:279-280
@@ -278,2 +278,4 @@
         RegRefs.erase(SubregReg);
+        if (!Keep)
+          KeepRegs.reset(SubregReg);
       }
----------------
I am surprised that we need to guard `KeepRegs.reset()` with an `if` here. But well it preserves some existing behavior so if it is somehow necessary to have the `if` here keep it.

================
Comment at: test/CodeGen/X86/pr27681.mir:8-11
@@ +7,6 @@
+
+  define i32 @main() {
+  entry:
+    ret i32 0
+  }
+
----------------
I tended to use the even shorter `define void @main() { ret void }` here.

================
Comment at: test/CodeGen/X86/pr27681.mir:16-22
@@ +15,9 @@
+name:            main
+alignment:       4
+exposesReturnsTwice: false
+hasInlineAsm:    false
+allVRegsAllocated: true
+isSSA:           false
+tracksRegLiveness: true
+tracksSubRegLiveness: false
+calleeSavedRegisters: [ '%bh', '%bl', '%bp', '%bpl', '%bx', '%di', '%dil',
----------------
This can probably be simplified. I think `alignment`, `exposesReturnsTwice`, `hasInlineAsm`, `isSSA`, `tracksSubRegLiveness`, `calleeSavedRegisters` have their default values and do not need to be mentioned.

================
Comment at: test/CodeGen/X86/pr27681.mir:26-38
@@ +25,15 @@
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       52
+  offsetAdjustment: 0
+  maxAlignment:    4
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 16
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+fixedStack:
----------------
Similarily here, I assume most of the info here can be omitted.

================
Comment at: test/CodeGen/X86/pr27681.mir:45-54
@@ +44,12 @@
+stack:
+  - { id: 0, type: spill-slot, offset: -52, size: 2, alignment: 2 }
+  - { id: 1, type: spill-slot, offset: -53, size: 1, alignment: 1 }
+  - { id: 2, type: spill-slot, offset: -24, size: 4, alignment: 4 }
+  - { id: 3, type: spill-slot, offset: -28, size: 4, alignment: 4 }
+  - { id: 4, type: spill-slot, offset: -44, size: 4, alignment: 4 }
+  - { id: 5, type: spill-slot, offset: -48, size: 4, alignment: 4 }
+  - { id: 6, type: spill-slot, offset: -32, size: 4, alignment: 4 }
+  - { id: 7, type: spill-slot, offset: -36, size: 4, alignment: 4 }
+  - { id: 8, type: spill-slot, offset: -50, size: 2, alignment: 2 }
+  - { id: 9, type: spill-slot, offset: -40, size: 4, alignment: 4 }
+body:             |
----------------
It looks like only `stack.1`, `stack.5` and `stack.6` are used. Maybe you can reduce and rename this to 3 stack objects?

================
Comment at: test/CodeGen/X86/pr27681.mir:60-70
@@ +59,13 @@
+
+    frame-setup PUSH32r killed %ebp, implicit-def %esp, implicit %esp
+    frame-setup PUSH32r killed %ebx, implicit-def %esp, implicit %esp
+    frame-setup PUSH32r killed %edi, implicit-def %esp, implicit %esp
+    frame-setup PUSH32r killed %esi, implicit-def %esp, implicit %esp
+    %esp = frame-setup SUB32ri8 %esp, 36, implicit-def dead %eflags
+    %eax = MOV32ri 1
+    %ebp = MOV32ri 2
+    %ebx = MOV32ri 3
+    %ecx = MOV32ri 4
+    %edi = MOV32ri 5
+    %edx = MOV32ri 6
+
----------------
do the contents of bb.0 actually matter for the test to work?

================
Comment at: test/CodeGen/X86/pr27681.mir:91
@@ +90,3 @@
+    ; as a replacement register.
+    ; CHECK-LABEL: main
+    ; CHECK: %cl = AND8rr %cl, killed %b
----------------
for stylistic reasons I would move `CHECK-LABEL` upwards and check for `CHECK-LABEL: name: main` next to the `name:` line.

================
Comment at: test/CodeGen/X86/pr27681.mir:96-114
@@ +95,21 @@
+    %edx = MOV32ri 0
+    JE_1 %bb.3, implicit %eflags
+
+  bb.2:
+    successors: %bb.3
+    liveins: %cl, %eax, %ebp, %esi
+
+    OR32mr %esp, 1, _, 8, _, killed %eax, implicit-def %eflags ; :: (store 4 into %stack.5)
+    %dl = SETNEr implicit %eflags, implicit-def %edx
+
+  bb.3:
+    liveins: %cl, %ebp, %edx, %esi
+
+    %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags
+    %esp = ADD32ri8 %esp, 36, implicit-def dead %eflags
+    %esi = POP32r implicit-def %esp, implicit %esp
+    %edi = POP32r implicit-def %esp, implicit %esp
+    %ebx = POP32r implicit-def %esp, implicit %esp
+    %ebp = POP32r implicit-def %esp, implicit %esp
+    RET 0, %eax
+
----------------
Have you tried removing the lower part (bb.2, bb.3) and just ending bb.1 with RETQ to simplify the testcase further?


http://reviews.llvm.org/D20456