[PATCH] D158724: [AArch64][LoopVectorize] Add truncated store values to list of types for widening

[Irina Dobrescu via Phabricator via llvm-commits]

Rin added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5727
       // Examine the stored values.
-      if (auto *ST = dyn_cast<StoreInst>(&I))
+      if (auto *ST = dyn_cast<StoreInst>(&I)) {
         T = ST->getValueOperand()->getType();
----------------
david-arm wrote:
> I wonder if we should also be asking if the store operand is loop invariant too? This would avoid tests changing such as lvm/test/Transforms/LoopVectorize/vplan-stress-test-no-explict-vf.ll. If the input is loop invariant then it's not really participating in the vector loop.
Discussed this and there is a trunc store in the loop

```
 %0 = trunc i64 %indvars.iv21 to i32
  store i32 %0, ptr %arrayidx, align 4
```


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5729
         T = ST->getValueOperand()->getType();
+        if (isa<TruncInst>(ST->getOperand(0))) {
+          auto *castTrunc = dyn_cast<CastInst>(ST->getOperand(0));
----------------
david-arm wrote:
> david-arm wrote:
> > I think you can write this more simply as
> > 
> > ```if (auto *Trunc = dyn_cast<TruncInst>(ST->getOperand(0)))
> >   T = Trunc->getSrcTy();
> > ```
> nit: Sorry @Rin, just one more thing. Perhaps for consistency it makes sense to also use `ST->getValueOperand()` even though it's the same thing?
I'll change it, no problem.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5730
+        if (isa<TruncInst>(ST->getOperand(0))) {
+          auto *castTrunc = dyn_cast<CastInst>(ST->getOperand(0));
+          T = castTrunc->getSrcTy();
----------------
david-arm wrote:
> nit: Normally variables in LLVM start with a capital, i.e. `CastTrunc`
Ah, my bad, I'll change that.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/truncate-type-widening.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -S < %s -passes=loop-vectorize -force-vector-interleave=1 -mtriple aarch64-linux-gnu -mattr=+sve -sve-tail-folding=disabled 2>&1 | FileCheck %s
+
----------------
david-arm wrote:
> It looks like we're still using tail-folding despite passing in `-sve-tail-folding=disabled`, which I think is because the vectoriser knows the trip count is low. Perhaps you can just remove the flag?
You're right, I'll take that flag out.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/truncate-type-widening.ll:96
+
+for.cond.cleanup.loopexit:                        ; preds = %for.body
+  br label %for.cond.cleanup
----------------
david-arm wrote:
> I think you can delete this block and just let everything jump directly to `%for.cond.cleanup`
Makes sense I'll do that and rewrite the blocks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158724