This is an archive of the discontinued LLVM Phabricator instance.

[LV] widenIntOrFpInduction. NFC.
ClosedPublic

Authored by SjoerdMeijer on Mar 24 2020, 4:35 AM.

Details

Summary

This untangles some of the logic in widenIntOrFpInduction, which was quite necessary IMHO as the different cases was extremely difficult to follow and read. So I've removed/replaced this with more straigth-line code, which I would like to do first before making some other functional changes. Most of the times I directly commit NFC patches, but here I would like to get feedback as this is a little bit of a rewrite.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Mar 24 2020, 4:35 AM
fhahn added a subscriber: fhahn.Mar 24 2020, 5:58 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1813

Instead of implicitly setting Step in the lambdas, would it be possible to have the closures return the step/IV they create and take it as argument if required?

1827–1828

Comment seems out of place now.

1851–1852

The comment seems a bit out of place now, should it be around line 1910?

1852–1854

it might be good to make the VF, Part and Step explicit parameters here (and potentially other places) to make things a bit more explicit at the call sites.

+1 to Florian's suggestions for lambda parameters and some explicit data flow.

Thanks for taking a look @fhahn ! Comments addressed.

gilr added a subscriber: gilr.Mar 24 2020, 10:27 AM
Ayal added inline comments.Mar 25 2020, 9:57 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1817–1826

Seems like it may be good to pass in ID.getStep() as an explicit parameter, as it forms the basis of the step being created.

1824
1832–1833

+1 about the first part of the original comment "If we haven't yet vectorized the induction variable..." seeming out of place here now.
But the remaining part that explains what this code is doing, would help here to describe what this lambda is for, including that comment about truncation.
Possibly also resurrecting the original comment about ScalarIV (changing "will be" to "is"):

// The scalar value to broadcast. This will be derived from the canonical
// induction variable.
1851–1852

+1
Some explanation what this lambda is for may be good here though. It basically creates the vector values from the scalar IV, in the absence of creating a vector IV.

1864

Is CreateScalarIVCode() really needed, instead of invoking

Value *ScalarIV = CreateScalarIV(Step);
CreateSplatIV(ScalarIV, Step);

directly, as done below before calling buildScalarSteps()?
(They are not folded together because CreateScalarIV() may change Step, right?)

1887

Can continue early-exiting by doing next

if (!NeedsScalarIV) {
  createVectorIntOrFpInductionPHI(ID, Step, EntryVal);
  return;
}

cleaning up the checks for NeedsScalarIV below.

SjoerdMeijer marked 2 inline comments as done.Mar 25 2020, 1:23 PM
SjoerdMeijer added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1864

Ha, that's funny: no, we don't need it. Probably I stared too long at this code to notice this duplication and it is indeed the same as:

Value *ScalarIV = CreateScalarIV(Step);
CreateSplatIV(ScalarIV, Step);

which was actually also my intention. You're exactly right about:

They are not folded together because CreateScalarIV() may change Step, right?)

I wanted to have a one-liner:

CreateSplatIV(CreateScalarIV(Step), Step);

but that's indeed not possible because Step may change.

1887

Ah, many thanks. That cleans up things even more and this function is becoming a real beauty with just straight-line code.

Many thanks for reviewing!
I think this has all comments addressed.

Ayal added inline comments.Mar 25 2020, 5:33 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1822

There's one more ID.getStep() >> Step

1882–1883

Not sure where this first "If an induction ... doesn't need to be widened" sentence belongs now, but it doesn't fit here.
The rest does fit before calling buildScalarStep(), which is also/firstly invoked above... better place it there?

  • replaced ID.getStep() -> Step
  • removed the sentence on line 1899 - 1900, and
  • moved the comments on lines 1900 - 1904 to 1889.
Ayal accepted this revision.Mar 26 2020, 3:06 AM

This looks good to me, thanks. The title should be more descriptive, e.g., "[LV] Refactor widenIntOrFpInduction, NFC".
Please wait a day or so to see if @fhahn or @samparker have further comments.

This revision is now accepted and ready to land.Mar 26 2020, 3:06 AM

Will do, and many thanks for all your reviews!

And just fyi, I've uploaded work-in-progress patch D76838 that builds on top of this refactoring.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 6:29 AM