[PATCH] D90173: [PowerPC] Exploit splat instruction xxsplti32dx in Power10

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9195
+  // If we lose precision, we use XXSPLTI32DX.
+  if (BVNIsConstantSplat  && (SplatBitSize == 64) && Subtarget.hasPrefixInstrs()) {
+    if(convertToNonDenormSingle(APSplatBits) &&
----------------
Slightly over 80 characters.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9204
+
+      uint32_t top = (uint32_t) ((APSplatBits.getZExtValue() & 0xFFFFFFFF00000000LL) >> 32);
+      uint32_t bot = (uint32_t) (APSplatBits.getZExtValue() & 0xFFFFFFFF00000000LL);
----------------
These lines are also a bit over 80 characters, and I think we prefer to capitalize the variables.

nit: Maybe we can go with something like `Hi` and `Lo`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9208
+
+      if (!top || !bot) {
+        // if either load is 0, then we should generate XXLXOR to set to 0
----------------
nit: I believe the braces in all of these ifs can be omitted. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9209
+      if (!top || !bot) {
+        // if either load is 0, then we should generate XXLXOR to set to 0
+        SplatNode = DAG.getTargetConstant(0, dl, MVT::v2i64);
----------------
nit: Capitalize the sentence and end with period.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9220
+      if (top) {
+        SplatNode = DAG.getNode(
+            PPCISD::XXSPLTI32DX, bot ? SplatNode : DAG.getUNDEF(MVT::v2i64), 
----------------
Might be a dumb question, but do you not need `dl` here?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9230
 
   if (!BVNIsConstantSplat || SplatBitSize > 32) {
     bool IsPermutedLoad = false;
----------------
Unintended change?


================
Comment at: llvm/test/CodeGen/PowerPC/p10-splatImm-CPload-pcrel.ll:17
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    plxv vs34, .LCPI0_0 at PCREL(0), 1
+; CHECK-NEXT:    xxlxor vs34, vs34, vs34
+; CHECK-LE-NEXT:    xxsplti32dx vs34, 1, 1081435463
----------------
Since the CHECKs are usually generated by `update_llc_test_checks.py` in this test case like stated on the top, I think the formatting of the CHECKs won't be outputted the way you currently have it the next time this test gets updated. 

It might be better to just run the script and have all the CHECKs be separated out. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90173/new/

https://reviews.llvm.org/D90173