[PATCH] D17850: Power9 - Implement byte comparison and count trailing zero instructions

Nemanja Ivanovic via llvm-commits llvm-commits at lists.llvm.org
Sat Mar 5 23:30:28 PST 2016


nemanjai marked an inline comment as done.

================
Comment at: lib/Target/PowerPC/PPC.td:154-159
@@ -153,2 +153,8 @@
                    [FeatureVSX]>;
+def FeatureCharCmp   :
+  SubtargetFeature<"char-cmp", "HasCharCmp", "true",
+                   "Enable the Character-Type compare instructions">;
+def FeatureCTTZ   :
+  SubtargetFeature<"cttz", "HasCTTZ", "true",
+                   "Enable count trailing zeros instructions">;
 
----------------
amehsan wrote:
> It will make the code more readable to have something in the name or description of the feature that indicates the feature was introduced in P9. I know we do not have it for many features, but as features are added, some extra clarity will be helpful.
I think we generally did not mention any of the processors a feature is implemented on because such features can become part of multiple chips. For example, FeatureLDBRX is available on Power7, but it's also available on the A2 chip.

That being said, I'm not terribly opposed to having P9 in the name, but I think I'll defer the final decision to @hfinkel.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:567
@@ -566,1 +566,3 @@
                            IIC_IntCompare>, isPPC64;
+  def CMPRB  : X_BF3_L1_RS5_RS5<31, 192, (outs crbitrc:$BF),
+                                 (ins u1imm:$L, g8rc:$rA, g8rc:$rB),
----------------
cycheng wrote:
> cycheng wrote:
> > These instructions are enclosed in "let PPC970_Unit = 1 in { ... }"
> > 
> > It looks like PPC970 indicating specific processor, i.e. G5, and it's purpose seems for PPCHazardRecognizer970 instruction scheduling implementation, so I don't know whether "let PPC970_Unit ..." is required for Power9 instructions or not.
> Should we move CMPRB to PPCInstrInfo.td, and provide CMPRB8 definition here? Because it looks like 32-bit instructions:
>   (RA)56:63
>   (RB)32:63
This is a good point. I initially implemented it that way and then removed it from the 32-bit definition. However, both the Hi and Lo range delimiters are in the lower 32-bits of the registers so this should be perfectly usable in 32-bit mode.
I'll add it there.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:567
@@ -566,1 +566,3 @@
                            IIC_IntCompare>, isPPC64;
+  def CMPRB  : X_BF3_L1_RS5_RS5<31, 192, (outs crbitrc:$BF),
+                                 (ins u1imm:$L, g8rc:$rA, g8rc:$rB),
----------------
nemanjai wrote:
> cycheng wrote:
> > cycheng wrote:
> > > These instructions are enclosed in "let PPC970_Unit = 1 in { ... }"
> > > 
> > > It looks like PPC970 indicating specific processor, i.e. G5, and it's purpose seems for PPCHazardRecognizer970 instruction scheduling implementation, so I don't know whether "let PPC970_Unit ..." is required for Power9 instructions or not.
> > Should we move CMPRB to PPCInstrInfo.td, and provide CMPRB8 definition here? Because it looks like 32-bit instructions:
> >   (RA)56:63
> >   (RB)32:63
> This is a good point. I initially implemented it that way and then removed it from the 32-bit definition. However, both the Hi and Lo range delimiters are in the lower 32-bits of the registers so this should be perfectly usable in 32-bit mode.
> I'll add it there.
I have added them to this section assuming it is sufficiently similar to these instructions from the standpoint of scheduling. However, I don't understand the details of how this information is used. Hopefully @hfinkel can provide some guidance here.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:570
@@ +569,3 @@
+                                 "cmprb $BF, $L, $rA, $rB", IIC_IntCompare>,
+                Requires<[HasCharCmp]>;
+  def CMPEQB : X_BF3_RS5_RS5<31, 224, (outs crbitrc:$BF),
----------------
amehsan wrote:
> any reason for different indentation of 'Requires' clause? I noticed that sometimes we use this different indentation for 'Requires' and sometimes not. Couldn't figure out justification for having inconsistent indentation.
I realize that there are some places where the "Requires" base class appears on the start of a line and it is aligned with the name of the instruction. However, since the definition of the instruction inherits from two classes (namely, `X_BF3_L1_RS5_RS5` and `Requires` in this case), I feel that both classes inherited should be aligned after the colon.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:626
@@ -615,3 +625,3 @@
                          [(set i64:$rA, (sra i64:$rS, (i32 imm:$SH)))]>, isPPC64;
-defm CNTLZD : XForm_11r<31, 58, (outs g8rc:$rA), (ins g8rc:$rS),
+defm CNTLZD : XForm_11r<31,  58, (outs g8rc:$rA), (ins g8rc:$rS),
                         "cntlzd", "$rA, $rS", IIC_IntGeneral,
----------------
amehsan wrote:
> I think you introduced an extra space unintentionally.
The intention was to align everything to the right of "58" to the newly added instruction (similarly to `CNTLZW` in the 32-bit instruction definition file). I know it's a rather unrelated change, I just thought it looked nicer all ligned up :).

================
Comment at: lib/Target/PowerPC/PPCInstrFormats.td:767
@@ +766,3 @@
+
+// Same as XForm_17 but with GPR's and new naming convention
+class X_BF3_RS5_RS5<bits<6> opcode, bits<10> xo, dag OOL, dag IOL,
----------------
cycheng wrote:
> amehsan wrote:
> > You can probably remove the comment. Does it help the reader to know this class is very similar to XForm_17?
> I think it's helpful : D
Me too. I think it's a quick reference for someone that is looking for a form to use and XForm_17 is almost right. Also, if we ever get around to renaming forms to have a common scheme, it can be helpful.

================
Comment at: lib/Target/PowerPC/PPCInstrFormats.td:770
@@ +769,3 @@
+                    string asmstr, InstrItinClass itin>
+         : I<opcode, OOL, IOL, asmstr, itin> {
+  bits<3> BF;
----------------
amehsan wrote:
> different indentation in here and the other class that you defined right before this.
That's a good point. I don't know how this one got out. I'll fix it. Thanks.


Repository:
  rL LLVM

http://reviews.llvm.org/D17850





More information about the llvm-commits mailing list