[PATCH] D79162: [Analysis] TTI: Add CastContextHint for getCastInstrCost

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Yeah, I think this is looking better. Thanks for the update. But I would be interested in the opinion of others too. It seems to get this information through to the costmodel and be more accurate than just presuming 'I' will be correct.

In D79162#2029088 <https://reviews.llvm.org/D79162#2029088>, @Pierre-vh wrote:

> Note that this patch doesn't support `Interleave` and `Reversed` in `getCastContextHint` - I personally don't know how to do that, so feedback is welcome.


I think that's fine of the moment. They are implemented as shufflevectors, so I don't think the extend and the load will usually combine. Essentially treating them as None should be OK, as far as I understand. And we can adjust that in the future if we need to.



================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:919
+  enum class CastContextHint : uint8_t {
+    None,          ///< No context is provided.
+    Normal,        ///< The cast is used with a normal load/store.
----------------
Considering how this is used, it looks like None would mean "This is not a load/strore", not "No context". I guess if we don't have I and we are not provided with anything better, we don't have any context. But in all other situations we at least try to calculate it from I.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:725
       // extending load exists on target.
       if (I && isa<LoadInst>(I->getOperand(0))) {
         EVT ExtVT = EVT::getEVT(Dst);
----------------
I feel like this should now be CCH == TTI::CastContextHint::Normal.

The other types of loads won't apply for the logic below.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6405
+    TTI::CastContextHint CCH = TTI::CastContextHint::None;
+    if (VF > 1) {
+      unsigned Opcode = I->getOpcode();
----------------
If the VF ==1 and the operand is a load/store, this should likely use Normal.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6406
+    if (VF > 1) {
+      unsigned Opcode = I->getOpcode();
+      // For Trunc, the "context" is the single user of the instruction, which
----------------
You either don't need unsigned Opcode = I->getOpcode(), or the variable can be used more throughout this function?


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

https://reviews.llvm.org/D79162