Page MenuHomePhabricator

[LV] Scalar Epilogue Lowering. NFC.
ClosedPublic

Authored by SjoerdMeijer on Jul 18 2019, 6:04 AM.

Details

Summary

This refactors boolean 'OptForSize' that was passed around in a lot of places
into isScalarEpilogueAllowed, which was already part of the CostModel. This
boolean controls folding of the tail loop, the scalar epilogue, into the main
loop but code-size reasons may not be the only reason to do this. Thus, this is
a first step to generalise this, and hence OptForSize has been renamed to
isScalarEpilogueAllowed.

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Jul 18 2019, 6:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 6:04 AM

Generally, the patch looks fine. I'd would prefer if someone who has contributed to VPlan to accept the patch.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
848 ↗(On Diff #210538)
4677 ↗(On Diff #210538)

Renaming CantVersionLoopWithOptForSize breaks API: Tools parsing the YAML output might match the string. I wouldn't rename these identifiers without a reason.

4795 ↗(On Diff #210538)

Using the getter method for the object's own field looks strange. Did you consider directly using IsScalarEpilogueAllowed?

Feedback addressed. Thanks for taking a look!

Looks to be a good change. Can we add a little more improvement to this patch --- adding more crispness in ORE message?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4683 ↗(On Diff #210790)

I understand that the existing code didn't properly reflect having -Os/-Oz but losing this, where applicable, isn't great. This being a debug message, I won't insist, since "performing code size checks" debug message is already printed.

4752 ↗(On Diff #210790)

Ideally, this is where we'd like to be crisp about whether the code was actually compiled under Os/Oz or vectorizer itself is forcing no scalar epilogue. If the flag is used, programmer has an option of using pragma or getting rid of the flag. If vectorizer is forcing, the only choice is using pragma.

In order for us to be able to do that, IsScalarEpilogueAllowed, when it is false, should come with reasoning behind it. May want to consider changing it to have enum value instead, e.g., ScalarEpilogueAllowed, ScalarEpilogueNotAllowedOptSize, ScalarEpilogueNotAllowedLowTripLoop.

Thanks for the suggestion: I have generalized isScalarEpilogueAllowed by introducing an enum to model different reasons why it might be allowed or not.

I have reverted most/all changes I made to the debug messages. I will look at that again when I will make actual functional changes in this area, which is as soon as this groundwork lands.

Just one comment from me.

@Meinersbur, you think your comments are addressed enough?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1290 ↗(On Diff #211088)

IsSomething sounds like a boolean value. Suggest something like ScalarEpilogueStatus, ScalarEpilogueSetting, etc.

Just one comment from me.

@Meinersbur, you think your comments are addressed enough?

Yes.

SjoerdMeijer retitled this revision from [LV] isScalarEpilogueAllowed. NFC. to [LV] Scalar Epilogue Lowering. NFC..

Many thanks for reviewing, very much appreciated!

Comments addressed: renamed member IsScalarEpilogueAllowed to ScalarEpilogueStatus

hsaito accepted this revision.Wed, Jul 24, 12:08 PM

LGTM

This revision is now accepted and ready to land.Wed, Jul 24, 12:08 PM
This revision was automatically updated to reflect the committed changes.