This is an archive of the discontinued LLVM Phabricator instance.

[LV] Merge floating point and integer induction widening code
ClosedPublic

Authored by mssimpso on Feb 21 2017, 9:34 AM.

Details

Summary

This patch merges the existing floating point induction variable widening code into the integer induction variable widening code, creating a single set of functions for both kinds of inductions. The primary motivation for doing this is to enable vector phi node creation for floating point induction variables.

Diff Detail

Repository
rL LLVM

Event Timeline

mssimpso created this revision.Feb 21 2017, 9:34 AM
mkuper edited edge metadata.Feb 21 2017, 4:59 PM

Thanks, this basically looks good, a few comments inline.

lib/Transforms/Vectorize/LoopVectorize.cpp
387 ↗(On Diff #89235)

Nothing changed here, you just moved it around, right?

400 ↗(On Diff #89235)

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?

test/Transforms/LoopVectorize/float-induction.ll
4 ↗(On Diff #89235)

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?

delena added inline comments.Feb 21 2017, 11:46 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
2467 ↗(On Diff #89235)

I suggest to simplify the expression:
IV->getType()->isIntegerTy() || IV != OldInduction

2501 ↗(On Diff #89235)

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.

2635 ↗(On Diff #89235)

I'm not 100% sure about the following comment:
For FP, the operation may be FADD or FSUB. It depends on reduction opcode.

mkuper added inline comments.Feb 22 2017, 12:29 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
2501 ↗(On Diff #89235)

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.

2635 ↗(On Diff #89235)

I think you're right. (You mean induction opcode, right?)

mssimpso added inline comments.Feb 22 2017, 6:24 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
387 ↗(On Diff #89235)

Right, I just moved it so I could reuse it.

400 ↗(On Diff #89235)

You're right, nice catch!

2467 ↗(On Diff #89235)

Yes that's much simpler. Thanks!

2635 ↗(On Diff #89235)

Ah, yes that's true. We'll need to get the step BinOp from the induction descriptor. Thanks!

test/Transforms/LoopVectorize/float-induction.ll
4 ↗(On Diff #89235)

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?

mkuper added inline comments.Feb 22 2017, 10:51 AM
test/Transforms/LoopVectorize/float-induction.ll
4 ↗(On Diff #89235)

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.

mssimpso added inline comments.Feb 22 2017, 10:54 AM
test/Transforms/LoopVectorize/float-induction.ll
4 ↗(On Diff #89235)

Great, that's what I was thinking as well.

mssimpso updated this revision to Diff 89392.Feb 22 2017, 11:44 AM
mssimpso marked 4 inline comments as done.

Addressed comments from Michael and Elena. Thanks!

  • Changed the constant helper to use signed values.
  • Simplified assert in widenIntOrFpInduction.
  • Used the induction opcode from the induction descriptor for the "AddOp" in buildScalarSteps and createVectorIntOrFPInductionPHI. I corrected one of the test cases I incorrectly updated the first time around. It now correctly uses fsub instead of fadd for the induction update.
  • Rebased new test case.
mkuper added inline comments.Feb 22 2017, 12:13 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
400 ↗(On Diff #89235)

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.)

test/Transforms/LoopVectorize/float-induction.ll
4 ↗(On Diff #89235)

Sorry, I meant for all test cases you changed, not just the new one.

mssimpso added inline comments.Feb 22 2017, 12:23 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
400 ↗(On Diff #89235)

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.

test/Transforms/LoopVectorize/float-induction.ll
4 ↗(On Diff #89235)

Ah, I see! Sorry for the confusion. I'll run the script over the tests I changed and rebase.

mkuper added inline comments.Feb 22 2017, 1:04 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
400 ↗(On Diff #89235)

Yeah, makes sense.

mssimpso updated this revision to Diff 89419.Feb 22 2017, 2:13 PM

Rebased tests.

mkuper accepted this revision.Feb 22 2017, 3:52 PM

LGTM

This revision is now accepted and ready to land.Feb 22 2017, 3:52 PM
delena added inline comments.Feb 22 2017, 11:06 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
4741 ↗(On Diff #89419)

Is the pointer induction really different from int and FP induction?
May be you can put everything under widenInduction?

mssimpso added inline comments.Feb 23 2017, 4:12 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
4741 ↗(On Diff #89419)

It's not, and I'm planning to work on this soon. Thanks!

delena added inline comments.Feb 23 2017, 4:29 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
4741 ↗(On Diff #89419)

I'd put them all in one patch and call the function widenInduction(). but it is not a stumbling-block anyway.

mssimpso added inline comments.Feb 23 2017, 5:00 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
4741 ↗(On Diff #89419)

Would you mind if we did this in a separate patch? I was trying to focus this one on floating-point, while keeping everything thing else as NFC as possible. Once committed, we can make sure no performance issues crop up for anyone before moving on to the pointer induction variables. The suggestion makes sense - I just would like to keep things small for review and codgen investigations should the need arise.

mssimpso added inline comments.Feb 24 2017, 9:34 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
4741 ↗(On Diff #89419)

Hi Elena,

I'm going to go ahead and commit this - assuming no issues arise over the weekend, I'll submit a patch to take care of the pointer inductions early next week, like you suggested.

This revision was automatically updated to reflect the committed changes.