Page MenuHomePhabricator

[LV] Let recordVectorLoopValueForInductionCast to check if IV was created from the cast.
ClosedPublic

Authored by a.elovikov on Feb 27 2018, 2:23 AM.

Details

Summary

It turned out to be error-prone to expect the callers to handle that - better to
leave the decision to this routine and make the required data to be explicitly
passed to the function.

This handles the case that was missed in the r322473 and fixes the assert
mentioned in PR36524.

Diff Detail

Repository
rL LLVM

Event Timeline

a.elovikov created this revision.Feb 27 2018, 2:23 AM
a.elovikov retitled this revision from [WIP][LV] Don't call recordVectorLoopValueForInductionCast for more newly-created IVs from a trunc. to [LV] Let recordVectorLoopValueForInductionCast to check if IV was created from the cast..
a.elovikov edited the summary of this revision. (Show Details)
a.elovikov added reviewers: dorit, mssimpso, Ayal.
Ayal resigned from this revision.Feb 27 2018, 12:01 PM
Ka-Ka added a subscriber: Ka-Ka.Mar 5 2018, 12:52 AM

Hopefully I can delegate the review to Diego...
Thanks for the fix, Andrei

Thanks for taking care of this, Andrei. Some comments below.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
573 ↗(On Diff #136115)

The previous documentation seems clearer to me but I could be missing some subtle detail.
Typo: remove ')'?

605 ↗(On Diff #136115)

It seems there is some given assumption that is not totally clear. The previous comments don't explicitly say anything about newly-created IVs. However, it seems that depending on the kind of EntryVal, we are able to know if the IV is an original or a newly-created one. Could you, please, elaborate the documentation a bit more in this regard and explain which kinds (Trunc) mean newly-created IV and why?

2501 ↗(On Diff #136115)

Move comment inside the 'if'?

llvm/test/Transforms/LoopVectorize/X86/pr36524.ll
7 ↗(On Diff #136115)

Is it necessary to check all the instructions in the loop body? Maybe we could remove some of them (e.g., icmp, br, ...) and strictly check whether the loop has been vectorized (vector.body) and the IV related instructions? Otherwise, this test might fail in the future due to changes unrelated to this fix.

a.elovikov added inline comments.Mar 14 2018, 6:10 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
573 ↗(On Diff #136115)

The reason I made this change is because all the code I've seen expects EntryVal to be either PHI or TruncInst without any other possibilities. It is also true in the places where the parameters to the calls to this routine are formed. I've added an assert to ensure this property won't break - but that also required this small correction to the doc string.

So the change in a few words: before - phi/trunc/anything else is possible, after: only phi/trunc are expected.

Thanks for noticing ")"!

605 ↗(On Diff #136115)

These assumptions come from the callers of this routine, namely buildScalarSteps/createVectorIntOrFPInductionPHI.

This routine only makes sense in that context (and that was before my change), so I don't think we need to document the real meaning of the EntryVal here - only how it affects the processing being done in this routine.

2501 ↗(On Diff #136115)

I briefly looked through this file, it seems more common to have the comment the way it is now. See line 2610 as an example (one-line if condition with the preceding comment).

llvm/test/Transforms/LoopVectorize/X86/pr36524.ll
7 ↗(On Diff #136115)

This is the output of the update_test_checks.py limited to a vector BB only that's why I'd prefer to leave it as-is. I also think that it's small enough and won't change frequently (and all the changes, if any, would lead to a simplification that would be obviously good).

dcaballe added inline comments.Mar 14 2018, 5:20 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
573 ↗(On Diff #136115)

Ok, thanks for the clarification.

605 ↗(On Diff #136115)

I think that a quick comment would be necessary, at least, to be more explicit on where to find further information. Something like what you just said: "This function is used in the context of 'buildScalarSteps' and 'createVectorIntOrFPInductionPHI', where X and Y is assumed."

2501 ↗(On Diff #136115)

I've been asked to do the same change in this file. I think it makes sense since you are actually describing what happens in the if-then branch. Someone without enough knowledge of this code might understand that the comment is a generic description. If you move it inside the 'if', there is no room for doubt. But this is just my suggestion.

Improve some comments.

dcaballe accepted this revision.Mar 15 2018, 10:45 AM

Thanks, Andrei! This fix LGTM but, please, wait for a few days before committing it to see if somebody else has any comments.

This revision is now accepted and ready to land.Mar 15 2018, 10:45 AM
This revision was automatically updated to reflect the committed changes.