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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a reviewer: mkuper.
rengolin added a comment.

In https://reviews.llvm.org/D29631#684674, @jonpa wrote:

> - getCmpSelInstrCost() has gotten a new default nullptr parameter for the Instruction. This is so that when the instruction is available, it can be passed. This is needed for SystemZ to estimate how many  instructions are needed in addition to the vector compare and vector select instructions (it depends on element widths). LoopVectorizer and CostModel passes the instruction, and elswhere it is just ignored. Target implementation has also gotten the (unused) argument in declaration and definition of the method.


This doesn't look bad, but maybe Matthew/Michael have a different plan for such problem. I'm personally ok with this.

> - BasicTTIImpl has gotten a new method getScalarizationOverhead(), that contains factored-out code from getArithmeticInstrCost(), so that the SystemZ (and potentially others) implementation can use it.

I think you could simplify that by implementing the "empty args" logic inside `getOperandsScalarizationOverhead`.

> - LoopVectorizer: Don't consider the compare in the loop latch (used by the conditional back-branch), to be vectorized. This doesn't make sense, it will always be scalar, right? Renato?

I'm not sure it has to be always scalar. If you have masked vector instructions, the latch could be a vector comparison. Or maybe I didn't get what the problem is. Can you give an example?

--renato



================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:334
+      // associated with one argument as a heuristic.
+      Cost += getScalarizationOverhead(VecTy, false, true);
+
----------------
This sounds like it should be implemented inside `getOperandsScalarizationOverhead`


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:711
+      return Cost;
+    }
+    else { // Called with a select instruction.
----------------
Code style, try clang-format.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.h:30
 
+  unsigned const LIBCALL_COST = 30;
+
----------------
Shouldn't this just be the cost of a call, rather than being a magic constant?


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.h:58
 
+  bool isFPVectorizationPotentiallyUnsafe() { return false; }
+
----------------
Where is this used?


https://reviews.llvm.org/D29631