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

[amehsan via llvm-commits]

amehsan added a comment.

just a few minor nits. I did not review the tests though. I am going to learn that part of the code today :)


================
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">;
 
----------------
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.

================
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),
----------------
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.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:592
@@ -583,1 +591,3 @@
+                        "cnttzw", "$rA, $rS", IIC_IntGeneral, []>,
+               Requires<[HasCTTZ]>;
 
----------------
same question here. 

================
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,
----------------
I think you introduced an extra space unintentionally.

================
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,
----------------
You can probably remove the comment. Does it help the reader to know this class is very similar to XForm_17?

================
Comment at: lib/Target/PowerPC/PPCInstrFormats.td:770
@@ +769,3 @@
+                    string asmstr, InstrItinClass itin>
+         : I<opcode, OOL, IOL, asmstr, itin> {
+  bits<3> BF;
----------------
different indentation in here and the other class that you defined right before this.


Repository:
  rL LLVM

http://reviews.llvm.org/D17850