Page MenuHomePhabricator

fix PPC64 SUBFC8 Defs list
ClosedPublic

Authored by vtjnash on May 30 2016, 9:34 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

vtjnash updated this revision to Diff 59012.May 30 2016, 9:34 PM
vtjnash retitled this revision from to fix PPC64 SUBFC8 Defs list.
vtjnash updated this object.
vtjnash set the repository for this revision to rL LLVM.
vtjnash added a subscriber: loladiro.

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);
+    }
   }
 }

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

vtjnash updated this revision to Diff 59083.May 31 2016, 9:57 AM
vtjnash updated this object.

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.

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 added inline comments.May 31 2016, 12:09 PM
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 marked an inline comment as done.Jun 1 2016, 9:13 AM
vtjnash added inline comments.
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%)
vtjnash updated this revision to Diff 59242.Jun 1 2016, 9:44 AM
vtjnash marked 3 inline comments as done.
hfinkel accepted this revision.Jun 1 2016, 12:31 PM
hfinkel edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Jun 1 2016, 12:31 PM

Do you need someone to commit this for you?

-Hal

  • Original Message -----

From: "Hal Finkel" <hfinkel@anl.gov>
To: vtjnash@gmail.com, hfinkel@anl.gov
Cc: amehsan@ca.ibm.com, "nemanja i ibm" <nemanja.i.ibm@gmail.com>, kfischer@college.harvard.edu
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

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

Hi Keno,

Please go ahead.

Thanks!

-Hal

  • Original Message -----

From: "Keno Fischer" <kfischer@college.harvard.edu>
To: vtjnash@gmail.com, hfinkel@anl.gov
Cc: amehsan@ca.ibm.com, "nemanja i ibm" <nemanja.i.ibm@gmail.com>, kfischer@college.harvard.edu
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

This revision was automatically updated to reflect the committed changes.