This is an archive of the discontinued LLVM Phabricator instance.

[VPlan][NFC] Introduce constructor for VPIteration
ClosedPublic

Authored by david-arm on Jan 29 2021, 5:24 AM.

Details

Summary

This patch adds constructors to VPIteration as a cleaner way of
initialising the struct and replaces existing constructions of
the form:

{Part, Lane}

with

VPIteration(Part, Lane)

This refactoring will be required in a later patch that adds more
members and functions to VPIteration.

Diff Detail

Event Timeline

david-arm created this revision.Jan 29 2021, 5:24 AM
david-arm requested review of this revision.Jan 29 2021, 5:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2021, 5:24 AM
sdesmalen added inline comments.Feb 2 2021, 1:52 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2143–2144

Just an observation for this patch in isolation: Writing {Part, Lane} would still cause the new constructor of VPIteration to be called, so this would be an unnecessary change. But given the following patch adds another operand, using VPIteration explicitly helps clarify the code.

llvm/lib/Transforms/Vectorize/VPlan.h
104

isFirstIteration might be more applicable, especially given that in D95139 it also needs to look for the lane kind.

david-arm updated this revision to Diff 320725.Feb 2 2021, 3:08 AM
  • Changed name from isNull to isFirstIteration
david-arm marked an inline comment as done.Feb 2 2021, 3:08 AM
sdesmalen added inline comments.Feb 2 2021, 3:27 AM
llvm/lib/Transforms/Vectorize/VPlan.h
101

From what I can see, most uses of VPIteration() in this patch are where it explicitly tries to get the first iteration. I think it's better to drop the implicit '0, 0' constructor, so that all VPIteration objects are explicitly constructed with a specific lane and part. Perhaps you can add a named method to return the first lane, e.g.

static VPIteration getFirstLane() { return VPIteration(0, 0); }
fhahn added inline comments.Feb 2 2021, 3:35 AM
llvm/lib/Transforms/Vectorize/VPlan.h
101

I also think it's better to just explicitly instantiate the VPIteration, rather than having the default constructor defaulting to the first lane. Not sure about adding a helper, because VPIteration(0, 0) is shorter than VPIteration::getFirstLane and as explicit, especially because all other places also pass lane & part explicitly.

david-arm updated this revision to Diff 320752.Feb 2 2021, 5:02 AM
david-arm retitled this revision from [VPlan][NFC] Introduce constructors for VPIteration to [VPlan][NFC] Introduce constructor for VPIteration.
david-arm edited the summary of this revision. (Show Details)
  • Removed default constructor VPIteration() and replaced uses with VPIteration(0, 0)
david-arm marked 2 inline comments as done.Feb 2 2021, 5:03 AM
david-arm added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
101

Hi @sdesmalen, hope you don't mind but I chose to go with @fhahn's suggestion of just using VPIteration(0, 0)

sdesmalen added inline comments.Feb 2 2021, 5:26 AM
llvm/lib/Transforms/Vectorize/VPlan.h
101

Sure I'm happy with that.

sdesmalen accepted this revision.Feb 2 2021, 5:27 AM

LGTM!

This revision is now accepted and ready to land.Feb 2 2021, 5:27 AM
fhahn accepted this revision.Feb 2 2021, 1:39 PM

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.