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

Matthew Simpson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 22 06:24:34 PST 2017


mssimpso added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:387
 
+/// A helper function that adds a 'fast' flag to floating point operations.
+static Value *addFastMathFlag(Value *V) {
----------------
mkuper wrote:
> Nothing changed here, you just moved it around, right?
Right, I just moved it so I could reuse it.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:400
+static Constant *getIntOrFpConstant(Type *Ty, unsigned C) {
+  return Ty->isIntegerTy() ? ConstantInt::get(Ty, C) : ConstantFP::get(Ty, C);
+}
----------------
mkuper wrote:
> One of the users of this for the int case was a getSigned(), and now it's a get(). Are you sure this is correct?
You're right, nice catch!


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2467
+             : true && "Primary induction variable must have an integer type");
 
   auto II = Legal->getInductionVars()->find(IV);
----------------
delena wrote:
> I suggest to simplify the expression:
> IV->getType()->isIntegerTy() || IV != OldInduction
Yes that's much simpler. Thanks!


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2635
+  } else {
+    AddOp = Instruction::FAdd;
+    MulOp = Instruction::FMul;
----------------
mkuper wrote:
> 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?)
Ah, yes that's true. We'll need to get the step BinOp from the induction descriptor. Thanks!


================
Comment at: test/Transforms/LoopVectorize/float-induction.ll:4
 ; RUN: opt < %s  -loop-vectorize -force-vector-interleave=2 -force-vector-width=1 -dce -instcombine -S | FileCheck --check-prefix VEC1_INTERL2 %s
+; RUN: opt < %s  -loop-vectorize -force-vector-interleave=1 -force-vector-width=2 -dce -simplifycfg -instcombine -S | FileCheck --check-prefix=VEC2_INTERL1_PRED_STORE %s
 
----------------
mkuper wrote:
> Did you run it through the update script? If you did, could you have the diff show the actual diff vs. running it with the old code?
Hey Michael, can you clarify what you mean here? I did use the script to help generate the checks (then moved the test into this file). You're just wanting to see the lines that are different with and without the patch?


https://reviews.llvm.org/D30211





More information about the llvm-commits mailing list