[PATCH] D148120: [InstCombine] Remove scalable get_active_lane_mask calls which are always false

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Thanks for this @MattDevereau! Looks like a nice improvement. I just have a few comments ...



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3052
+    // which always return all false predicates in certain scalable loops
+    if (!II->getType()->isScalableTy())
+      break;
----------------
I think the following optimisation should also work for fixed-width vectors, right? It just means that in your `MinVScaleElts` calculations below you wouldn't need to query the vscale minimum.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3064
+
+    auto Phi = dyn_cast<PHINode>(Idx);
+    if (!Phi)
----------------
I wonder if this is the canonical form or not? For example, I don't know if the operands would ever be swapped so the PHI node is on the right? Perhaps in order to make this more robust you could do something like:

    Value *AddOp1, *AddOp2;
    if (!match(Op0, m_Add(m_Value(AddOp1), m_Value(AddOp2))))
      break;

    Value *NonPhi = AddOp2;
    PHINode *Phi = dyn_cast<PHINode>(AddOp1);
    if (!Phi && (Phi = dyn_cast<PHINode>(AddOp2)))
      NonPhi = AddOp1;

    if (!Phi)
      break;

Not saying this is the best code obviously, but hopefully it helps to explain what I mean.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3068
+
+    ConstantInt *PhiOp0;
+    if (!match(Phi->getOperand(0), m_ConstantInt(PhiOp0)) ||
----------------
I think this condition needs to be a bit stronger, i.e. check the PHI has only two operands and that one incoming value comes from this block, then look for the other incoming value. The problem is that this might not be a loop, but instead just be a block with two predecessors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148120