[PATCH] D76686: [LV] widenIntOrFpInduction. NFC.

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer marked 2 inline comments as done.
SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1859
+
+  auto CreateScalarIVCode = [&](Value *Step) {
+    Value *ScalarIV = CreateScalarIV(Step);
----------------
Ayal wrote:
> Is CreateScalarIVCode() really needed, instead of invoking
> 
> ```
> Value *ScalarIV = CreateScalarIV(Step);
> CreateSplatIV(ScalarIV, Step);
> ```
> directly, as done below before calling buildScalarSteps()?
> (They are not folded together because CreateScalarIV() may change Step, right?)
Ha, that's funny: no, we don't need it. Probably I stared too long at this code to notice this duplication and it is indeed the same as:

  Value *ScalarIV = CreateScalarIV(Step);
  CreateSplatIV(ScalarIV, Step);

which was actually also my intention. You're exactly right about:

> They are not folded together because CreateScalarIV() may change Step, right?)

I wanted to have a one-liner:

  CreateSplatIV(CreateScalarIV(Step), Step);

but that's indeed not possible because Step may change. 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1883
+  auto NeedsScalarIV = needsScalarInduction(EntryVal);
+
+  // Try to create a new independent vector induction variable. If we can't
----------------
Ayal wrote:
> Can continue early-exiting by doing next
> 
> ```
> if (!NeedsScalarIV) {
>   createVectorIntOrFpInductionPHI(ID, Step, EntryVal);
>   return;
> }
> ```
> cleaning up the checks for NeedsScalarIV below.
Ah, many thanks. That cleans up things even more and this function is becoming a real beauty with just straight-line code.


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

https://reviews.llvm.org/D76686