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

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau marked 3 inline comments as done.
MattDevereau added inline 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;
----------------
david-arm wrote:
> 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.
I was under the impression this issue only appeared because of the vscale/vectorisation factor values stopping llvm from recognising everything as constants. There is `get_active_lane_mask` logic in `ConstantFolding.cpp`, however until I have the source code of this problem which I can test with fixed-width vectors, or run the source of this problem with D148010 applied it's hard to say if fixed-width vectors are really an issue.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3064
+
+    auto Phi = dyn_cast<PHINode>(Idx);
+    if (!Phi)
----------------
david-arm wrote:
> 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.
I've replaced this with some logic based on a call to `matchSimpleRecurrence` which should prove with a bit more tightness that it's actually a loop


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3068
+
+    ConstantInt *PhiOp0;
+    if (!match(Phi->getOperand(0), m_ConstantInt(PhiOp0)) ||
----------------
david-arm wrote:
> 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.
I've replaced this with some logic based on a call to matchSimpleRecurrence which should prove with a bit more tightness that it's actually a loop


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148120