[PATCH] D30211: [LV] Merge floating point and integer induction widening code

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2501
+  } else {
+    Step = cast<SCEVUnknown>(ID.getStep())->getValue();
+  }
----------------
delena wrote:
> ID.getStep() should already be SCEVUnknown if it is not scevable.
> 
> I think that the following code should work:
>   const SCEV *Step = ID.getStep();
>   if (PSE.getSE()->isSCEVable(IV->getType())) {
>     SCEVExpander Exp(*PSE.getSE(), DL, "induction");
>     Step = Exp.expandCodeFor(Step, ID.getStep()->getType(),
>                              LoopVectorPreHeader->getTerminator());
>    }
> 
> And I think you should use IV->getType() instead of ID.getStep()->getType(), while calling to expandCodeFor(), it will expand/truncate if needed.
> 
Step needs to be a Value, not a SCEV, hence the else.
(Perhaps you've read it as "cast<SCEVUnknown>(ID.getStep()->getValue());", not "cast<SCEVUnknown>(ID.getStep())->getValue()"?)

As to the other change - I think this patch is trying to be NFC for ints, so I think I'd prefer we not change that in this patch.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2635
+  } else {
+    AddOp = Instruction::FAdd;
+    MulOp = Instruction::FMul;
----------------
delena wrote:
> I'm not 100% sure about the following comment:
> For FP, the operation may be FADD or FSUB. It depends on reduction opcode. 
I think you're right. (You mean induction opcode, right?)


https://reviews.llvm.org/D30211