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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3237
+    BinaryOperator *BO;
+    Value *L, *R;
+    if (!matchSimpleRecurrence(Phi, BO, L, R) ||
----------------
I think 'L' and 'R' are a bit misleading here because they are actually 'Start' and 'Step', whereas on reading the current code it sounds like 'Left' and 'Right'. Can you rename them please?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3242
+
+    ConstantInt *PhiOp0 = dyn_cast<ConstantInt>(L);
+    if (!PhiOp0 || !PhiOp0->isZero())
----------------
Again, perhaps rename to `StartC`?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3246
+    ConstantInt *ShlValue;
+    if (!match(Vf, m_Shl(m_VScale(), m_ConstantInt(ShlValue))))
+      break;
----------------
In theory, Vf and 'Step' (or 'R' in the current code) should be identical. If not, then we can't apply the optimisation. I'm thinking of a case like this:

  loop:
    %idx = phi i64 [ 0, %preheader ], [ %idx.inc, %loop ]
    ...
    %idx.new = add i64 %idx, %vf
    %idx.inc = add i64 %idx, %other_thing
    %pred = call <vscale x 16 x i1> @llvm.active.get.lane.mask.nxv16i1.i64(%idx.new, %limit)

So I think you need to check that `Vf == Step` somehow.


================
Comment at: llvm/test/Transforms/InstCombine/get-active-lane-mask.ll:5
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
+
----------------
I think you either have to make this a generic test without the "target triple" or move this test into a AArch64 directory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148120