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

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added inline comments.


================
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:
> mssimpso wrote:
> > 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!
> So you're saying the get() on line 2592 should have been a getSigned() too?
> (Although I'm not sure it matters, for that case.)
Both of the original uses (get and getSigned) could only ever have values greater than one (and much less than the max int64_t) so I don't think it makes a difference either way. If we go with a helper function here, I think the signed version is less confusing.


================
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:
> mssimpso wrote:
> > mkuper wrote:
> > > mssimpso wrote:
> > > > 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?
> > > Yes, otherwise it's kind of hard to see what changed.
> > > I suggest running the script over the test with the existing code, committing that test, and rebasing this patch on that. That way we can actually see what happened.
> > Great, that's what I was thinking as well.
> Sorry, I meant for all test cases you changed, not just the new one.
Ah, I see! Sorry for the confusion. I'll run the script over the tests I changed and rebase.


https://reviews.llvm.org/D30211