fix PPC64 SUBFC8 Defs list

[vtjnash]

Summary

fix https://llvm.org/bugs/show_bug.cgi?id=27943

[nemanjai]

First off, I'd like to thank you for identifying and looking into this issue.

A couple of comments here:

1. Please post reviews with a full context diff rather than with just the default 3 lines of context.
2. Please select reviewers for patches. Hal Finkel owns the PPC back end, so at least he should be on the list of reviewers.
3. I don't think the correct approach is to change the definition of this instruction to no longer implicitly define "CARRY". The correct approach would probably be for it to implicitly define both of the special registers that it sets (CARRY, CR0).
4. You opened the PR with a sample test case. It is probably appropriate to add that test case to the repository with the patch that fixes the issue.

Also, I was not able to reproduce the issue with the default invocation of llc (it requires -O1 for me). And the attached test case still fails with this patch (although with a different message).
Finally, I think you've identified an issue in the PPC back end with the definition of record-form instructions similar to this. I'm hoping others can weigh in on this as well, but I think the more appropriate approach would be something like the following (with which the attached test case passes):

Index: lib/Target/PowerPC/PPCInstr64Bit.td
===================================================================
--- lib/Target/PowerPC/PPCInstr64Bit.td (revision 271231)
+++ lib/Target/PowerPC/PPCInstr64Bit.td (working copy)
@@ -514,11 +514,11 @@ let Defs = [CARRY] in {
 def SUBFIC8: DForm_2< 8, (outs g8rc:$rD), (ins g8rc:$rA, s16imm64:$imm),
                      "subfic $rD, $rA, $imm", IIC_IntGeneral,
                      [(set i64:$rD, (subc imm64SExt16:$imm, i64:$rA))]>;
+}
 defm SUBFC8 : XOForm_1r<31, 8, 0, (outs g8rc:$rT), (ins g8rc:$rA, g8rc:$rB),
                         "subfc", "$rT, $rA, $rB", IIC_IntGeneral,
-                        [(set i64:$rT, (subc i64:$rB, i64:$rA))]>,
+                        [(set i64:$rT, (subc i64:$rB, i64:$rA))], [CARRY]>,
                         PPC970_DGroup_Cracked;
-}
 defm SUBF8 : XOForm_1r<31, 40, 0, (outs g8rc:$rT), (ins g8rc:$rA, g8rc:$rB),
                        "subf", "$rT, $rA, $rB", IIC_IntGeneral,
                        [(set i64:$rT, (sub i64:$rB, i64:$rA))]>;
Index: lib/Target/PowerPC/PPCInstrInfo.td
===================================================================
--- lib/Target/PowerPC/PPCInstrInfo.td  (revision 271231)
+++ lib/Target/PowerPC/PPCInstrInfo.td  (working copy)
@@ -847,15 +847,16 @@ multiclass XForm_11r<bits<6> opcode, bits<10> xo,
 
 multiclass XOForm_1r<bits<6> opcode, bits<9> xo, bit oe, dag OOL, dag IOL,
                     string asmbase, string asmstr, InstrItinClass itin,
-                    list<dag> pattern> {
+                    list<dag> pattern, list<Register> AppendDefs = []> {
   let BaseName = asmbase in {
     def NAME : XOForm_1<opcode, xo, oe, OOL, IOL,
                        !strconcat(asmbase, !strconcat(" ", asmstr)), itin,
                        pattern>, RecFormRel;
-    let Defs = [CR0] in
     def o    : XOForm_1<opcode, xo, oe, OOL, IOL,
                        !strconcat(asmbase, !strconcat(". ", asmstr)), itin,
-                       []>, isDOT, RecFormRel;
+                       []>, isDOT, RecFormRel {
+      let Defs = !listconcat([CR0], AppendDefs);
+    }
   }
 }

[vtjnash]

I don't think the correct approach is to change the definition of this instruction to no longer implicitly define "CARRY". The correct approach would probably be for it to implicitly define both of the special registers that it sets (CARRY, CR0).

It's hard to see in the diff above, but I changed "XOForm_1r" to "XOForm_1rc", where the added "c" seems to mean CARRY and so I'm a bit confused why the two patches don't have the same effect.

You opened the PR with a sample test case. It is probably appropriate to add that test case to the repository with the patch that fixes the issue.

OK

[nemanjai]

In D20802#444480, @vtjnash wrote:

I don't think the correct approach is to change the definition of this instruction to no longer implicitly define "CARRY". The correct approach would probably be for it to implicitly define both of the special registers that it sets (CARRY, CR0).

It's hard to see in the diff above, but I changed "XOForm_1r" to "XOForm_1rc", where the added "c" seems to mean CARRY and so I'm a bit confused why the two patches don't have the same effect.

Ah OK, I'm sorry I must have missed the c at the end. And since the change looked so simple, I just changed it manually and missed the different instruction form you used. I apologize for this. I think this patch is fine with the addition of the test case.

But in retrospect, the way we implemented these instruction forms is conducive to errors such as the bug you fixed. Perhaps if we had implemented it initially as in the patch I added to the comment above, the additional Def would be explicit and more clearly visible.

Note that the patch I attached was just something I whipped together quickly and it's missing the "let Defs = AppendDefs" in the "NAME" def.

In any case, thanks again for fixing this.

[loladiro]

I think this patch is fine with the addition of the test case.

Is that a LGTM, or shall we wait for Hal to sign off?

[hfinkel]

lib/Target/PowerPC/PPCInstrInfo.cpp

1798 ↗

(On Diff #59083)

Please include a comment in the assert, for example:

assert(MI->definesRegister(PPC::CR0) && "Record-form instruction does not define cr0?");

test/CodeGen/PowerPC/opt-sub-inst-cr0-live.ll

1 ↗

(On Diff #59083)

Please convert this to an MIR-level test. This should make it more stable.

Run this:

llc -stop-after=machine-sink -mtriple=powerpc64-unknown-linux-gnu < test/CodeGen/PowerPC/opt-sub-inst-cr0-live.ll > test/CodeGen/PowerPC/opt-sub-inst-cr0-live.mir 2>&1

and then the test line can contain the RUN line:

RUN: llc -start-after=machine-sink -stop-after=peephole-opts -mtriple=powerpc64-unknown-linux-gnu -o - %s | FileCheck %s

we already have a few other MIR-level tests (test/CodeGen/PowerPC/*.mir).

[vtjnash]

test/CodeGen/PowerPC/opt-sub-inst-cr0-live.ll

1 ↗

(On Diff #59083)

There seems to be an issue with the yaml writer, but deleting the offending parenthetical seems to work. I'll update the diff with the MIR test

error: test/CodeGen/PowerPC/opt-sub-inst-cr0-live.mir:90:39: unexpected character '/'
    successors: %bb.1.loop(0x80000000 / 0x80000000 = 100.00%)

[hfinkel]

LGTM, thanks!

[hfinkel]

Do you need someone to commit this for you?

-Hal

• Original Message -----

From: "Hal Finkel" <[e-mail]>
To: [e-mail], [e-mail]
Cc: [e-mail], "nemanja i ibm" <[e-mail]>, [e-mail]
Sent: Wednesday, June 1, 2016 2:32:01 PM
Subject: Re: [PATCH] D20802: fix PPC64 SUBFC8 Defs list

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

LGTM, thanks!

Repository:

rL LLVM

http://reviews.llvm.org/D20802

[loladiro]

@vtjnash does not have commit access, so feel free to commit. Otherwise I can as well.

[hfinkel]

Hi Keno,

Please go ahead.

Thanks!

-Hal

• Original Message -----

From: "Keno Fischer" <[e-mail]>
To: [e-mail], [e-mail]
Cc: [e-mail], "nemanja i ibm" <[e-mail]>, [e-mail]
Sent: Wednesday, June 1, 2016 2:34:55 PM
Subject: Re: [PATCH] D20802: fix PPC64 SUBFC8 Defs list

loladiro added a comment.

@vtjnash does not have commit access, so feel free to commit.
Otherwise I can as well.

Repository:

rL LLVM

http://reviews.llvm.org/D20802

llvm/trunk/lib/Target/PowerPC/PPCInstr64Bit.td

 def ADDIS8 : DForm_2<15, (outs g8rc:$rD), (ins g8rc_nox0:$rA, s17imm64:$imm),
                      "addis $rD, $rA, $imm", IIC_IntSimple,
                      [(set i64:$rD, (add i64:$rA, imm16ShiftedSExt:$imm))]>;
 
 let Defs = [CARRY] in {
 def SUBFIC8: DForm_2< 8, (outs g8rc:$rD), (ins g8rc:$rA, s16imm64:$imm),
                      "subfic $rD, $rA, $imm", IIC_IntGeneral,
                      [(set i64:$rD, (subc imm64SExt16:$imm, i64:$rA))]>;
-defm SUBFC8 : XOForm_1r<31, 8, 0, (outs g8rc:$rT), (ins g8rc:$rA, g8rc:$rB),
+}
+defm SUBFC8 : XOForm_1rc<31, 8, 0, (outs g8rc:$rT), (ins g8rc:$rA, g8rc:$rB),
                         "subfc", "$rT, $rA, $rB", IIC_IntGeneral,
                         [(set i64:$rT, (subc i64:$rB, i64:$rA))]>,
                         PPC970_DGroup_Cracked;
-}
 defm SUBF8 : XOForm_1r<31, 40, 0, (outs g8rc:$rT), (ins g8rc:$rA, g8rc:$rB),
                        "subf", "$rT, $rA, $rB", IIC_IntGeneral,
                        [(set i64:$rT, (sub i64:$rB, i64:$rA))]>;
 defm NEG8    : XOForm_3r<31, 104, 0, (outs g8rc:$rT), (ins g8rc:$rA),
                         "neg", "$rT, $rA", IIC_IntSimple,
                         [(set i64:$rT, (ineg i64:$rA))]>;
 let Uses = [CARRY] in {
 let isCommutable = 1 in

llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.cpp

if (NewDesc.ImplicitDefs)
                          MachineOperand::CreateReg(*ImpDefs, true, true));
     if (NewDesc.ImplicitUses)
       for (const MCPhysReg *ImpUses = NewDesc.getImplicitUses();
            *ImpUses; ++ImpUses)
         if (!MI->readsRegister(*ImpUses))
           MI->addOperand(*MI->getParent()->getParent(),
                          MachineOperand::CreateReg(*ImpUses, false, true));
   }
+  assert(MI->definesRegister(PPC::CR0) &&
+         "Record-form instruction does not define cr0?");
 
   // Modify the condition code of operands in OperandsToUpdate.
   // Since we have SUB(r1, r2) and CMP(r2, r1), the condition code needs to
   // be changed from r2 > r1 to r1 < r2, from r2 < r1 to r1 > r2, etc.
   for (unsigned i = 0, e = PredsToUpdate.size(); i < e; i++)
     PredsToUpdate[i].first->setImm(PredsToUpdate[i].second);
 
   for (unsigned i = 0, e = SubRegsToUpdate.size(); i < e; i++)

llvm/trunk/test/CodeGen/PowerPC/opt-sub-inst-cr0-live.mir

+# RUN: llc -start-after=machine-sink -stop-after=peephole-opts -mtriple=powerpc64-unknown-linux-gnu -o /dev/null %s 2>&1 | FileCheck %s
+
+--- |
+  ; ModuleID = '<stdin>'
+  source_filename = "<stdin>"
+  target datalayout = "E-m:e-i64:64-n32:64"
+  target triple = "powerpc64-unknown-linux-gnu"
+
+  ; Function Attrs: nounwind readnone
+  declare i128 @llvm.cttz.i128(i128, i1) #0
+
+  define void @fn1(i128, i128, i1) {
+  top:
+    br label %loop
+
+  loop:                                             ; preds = %loop, %top
+    %v = phi i128 [ %3, %loop ], [ %0, %top ]
+    %u = phi i128 [ %3, %loop ], [ %1, %top ]
+    %s = sub i128 %v, %u
+    %3 = call i128 @llvm.cttz.i128(i128 %s, i1 false)
+    br label %loop
+  }
+
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**) #1
+
+  attributes #0 = { nounwind readnone }
+  attributes #1 = { nounwind }
+
+...
+---
+name:            fn1
+alignment:       2
+exposesReturnsTwice: false
+hasInlineAsm:    false
+allVRegsAllocated: false
+isSSA:           true
+tracksRegLiveness: true
+tracksSubRegLiveness: false
+registers:
+  - { id: 0, class: g8rc }
+  - { id: 1, class: g8rc }
+  - { id: 2, class: g8rc }
+  - { id: 3, class: g8rc }
+  - { id: 4, class: g8rc }
+  - { id: 5, class: g8rc }
+  - { id: 6, class: g8rc }
+  - { id: 7, class: g8rc }
+  - { id: 8, class: g8rc }
+  - { id: 9, class: g8rc }
+  - { id: 10, class: g8rc }
+  - { id: 11, class: g8rc }
+  - { id: 12, class: g8rc }
+  - { id: 13, class: g8rc }
+  - { id: 14, class: g8rc }
+  - { id: 15, class: g8rc_and_g8rc_nox0 }
+  - { id: 16, class: g8rc_and_g8rc_nox0 }
+  - { id: 17, class: g8rc }
+  - { id: 18, class: g8rc }
+  - { id: 19, class: g8rc }
+  - { id: 20, class: g8rc }
+  - { id: 21, class: g8rc }
+  - { id: 22, class: g8rc }
+  - { id: 23, class: g8rc }
+  - { id: 24, class: g8rc }
+  - { id: 25, class: crrc }
+  - { id: 26, class: g8rc_and_g8rc_nox0 }
+  - { id: 27, class: g8rc_and_g8rc_nox0 }
+liveins:
+  - { reg: '%x3', virtual-reg: '%6' }
+  - { reg: '%x4', virtual-reg: '%7' }
+  - { reg: '%x5', virtual-reg: '%8' }
+  - { reg: '%x6', virtual-reg: '%9' }
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+body:             |
+  bb.0.top:
+    successors: %bb.1.loop
+    liveins: %x3, %x4, %x5, %x6
+
+    %9 = COPY %x6
+    %8 = COPY %x5
+    %7 = COPY %x4
+    %6 = COPY %x3
+    %14 = COPY %9
+    %13 = COPY %8
+    %12 = COPY %7
+    %11 = COPY %6
+    %21 = LI8 128
+    %23 = LI8 64
+
+  bb.1.loop:
+    successors: %bb.2.loop, %bb.4
+
+    %0 = PHI %11, %bb.0.top, %4, %bb.3.loop
+    %1 = PHI %12, %bb.0.top, %5, %bb.3.loop
+    %2 = PHI %13, %bb.0.top, %4, %bb.3.loop
+    %3 = PHI %14, %bb.0.top, %5, %bb.3.loop
+    %15 = SUBFC8 %3, %1, implicit-def %carry
+    %16 = SUBFE8 %2, %0, implicit-def dead %carry, implicit %carry
+    %17 = ADDI8 %16, -1
+    %18 = ADDI8 %15, -1
+    %19 = ANDC8 killed %17, %16
+    %20 = ANDC8 killed %18, %15
+    %22 = CNTLZD killed %19
+    %24 = CNTLZD killed %20
+    %25 = CMPLDI %15, 0
+    BCC 76, %25, %bb.2.loop
+    ; CHECK: SUBFC8o %3, %1, implicit-def %carry, implicit-def %cr0
+    ; CHECK: COPY killed %cr0
+    ; CHECK: BCC
+
+  bb.4:
+    successors: %bb.3.loop
+
+    %27 = SUBF8 %24, %23
+    B %bb.3.loop
+
+  bb.2.loop:
+    successors: %bb.3.loop
+
+    %26 = SUBF8 %22, %21
+
+  bb.3.loop:
+    successors: %bb.1.loop
+
+    %5 = PHI %26, %bb.2.loop, %27, %bb.4
+    %4 = LI8 0
+    B %bb.1.loop
+
+...