[PATCH] D29631: SystemZTargetTransformInfo cost functions and some common code changes

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:269
+        else
+          NumStores++;
       }
----------------
uweigand wrote:
> This seems to partially duplicate the logic in getMemoryOpCost ... is there a way to unify those?
Yes - replaced it with a call to getMemoryOpCost() which returns the number of instructions just the same.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:343
+        Opcode == Instruction::AShr || Opcode == Instruction::Or) {
+      return NumVectors;
+    }
----------------
uweigand wrote:
> Shouldn't we have And and Xor here as well?
No - And and Xor are 'Legal', while the ones handled here are 'Custom'. Only for 'Custom' is this needed, since the default implementation then assumes it is twice as expensive, while it is actually not on z13.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:397
+    // Or requires one instruction, although it has custom handling for i64.
+    if (Opcode == Instruction::Or)
+      return 1;
----------------
uweigand wrote:
> Likewise ...
(same reason as above)


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:486
+        else //  Src is <16 x i64>
+          return (DstScalarBits == 32 ? 4 : (DstScalarBits == 16 ? 6 : 7));
+      }
----------------
uweigand wrote:
> Can this not be handled generically via getElSizeLog2Diff like below for the unpack case?
Computed by means of a loop instead. Special case also handled.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:531
+            { ISD::SIGN_EXTEND, MVT::v16i32, 12},
+            { ISD::SIGN_EXTEND, MVT::v16i64, 23},
+          };
----------------
uweigand wrote:
> These tables look odd ... but I guess if they accurately reflect the cost of the code that is currently being generated, this is fine with me until we improve codegen.
Yes - that was the intent.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:621
+      return (SrcScalarBits <= 64 ?
+              (SrcScalarBits >= 32 ? 1 : 2 /*i8/i16 extend*/) : LIBCALL_COST);
+    
----------------
uweigand wrote:
> When do we get libcalls?  At least with z196 and above this should never happen, so we might want to take the ISA level into account here.
This was for i128, which shouldn't be there I suppose, so I removed it. You get a libcall if you generate a function like that, but I have not seen it in benchmarks.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:737
+          if (CmpScalarBits == 64 && SelScalarBits == 16 && VF == 16)
+            PackCost -= 2; // Minor adjustment
+        }
----------------
uweigand wrote:
> This needs more explanation why this adjustment is needed in this case (and only in this case).
I refactored the cost computation for a vector truncation into a new function, used both for vector truncate and vector select, where the selected element type is smaller than the compared elements.


https://reviews.llvm.org/D29631