Page MenuHomePhabricator

[VPlan] Add getLiveInIRValue accessor to VPValue.
ClosedPublic

Authored by fhahn on Nov 29 2020, 1:31 PM.

Details

Summary

This patch adds a new getLiveInIRValue accessor to VPValue, which
returns the underlying value, if the VPValue is defined outside of
VPlan. This is required to handle scalars in VPTransformState, which
requires dealing with scalars defined outside of VPlan.

We can simply check VPValue::Def to determine if the value is defined
inside a VPlan.

Diff Detail

Event Timeline

fhahn created this revision.Nov 29 2020, 1:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2020, 1:31 PM
fhahn requested review of this revision.Nov 29 2020, 1:31 PM
Ayal added a comment.Jan 5 2021, 4:35 AM

This currently requires giving access to VPValue to those classes, so we
can call the protected constructor. But most of them can be removed in
the future, once the remaining single value def recipes inherit from
VPValue.

Such access seems unneeded as commented below; but in general may have been better to handle these remaining recipes first and avoid multiple befriendings, albeit temporary.

llvm/lib/Transforms/Vectorize/VPlanValue.h
64

As explained above, Def is null iff this VPValue is a "live-in", i.e., defined outside the loop (and used inside the loop).
So getOutOfScopeIRValue() (getLiveInValue()?) could simply return (getDef()? getUnderlyingValue() : nullptr), or its users could do so themselves directly?

fhahn updated this revision to Diff 314583.Jan 5 2021, 6:02 AM

Thanks for taking a look!

This currently requires giving access to VPValue to those classes, so we
can call the protected constructor. But most of them can be removed in
the future, once the remaining single value def recipes inherit from
VPValue.

Such access seems unneeded as commented below; but in general may have been better to handle these remaining recipes first and avoid multiple befriendings, albeit temporary.

Yes, originally this was not checking for VPDef, because when I posted the patch initially I did not want to block it on completing the transition. But that is done now, so just checking Def is much simpler.

I updated the code to just check VPDef (no need to befriend classes) and removed the function to getLiveInIRValue.

fhahn added inline comments.Jan 5 2021, 6:04 AM
llvm/lib/Transforms/Vectorize/VPlanValue.h
64

I did that and updated the function name. I think it is still worth to have it as a separate function, because there will be a few users in the following patches. Also, I think granting access to live-in IR values makes sense in general, even once we push back/reduce uses of getUnderlyingValue elsewhere.

fhahn updated this revision to Diff 314584.Jan 5 2021, 6:11 AM

Add assert to make sure getLiveInIRValue is only called for 'live-in' VPValues.

fhahn retitled this revision from [VPlan] Add getOutOfScopeIRValue accessor to VPValue. to [VPlan] Add getLiveInIRValue accessor to VPValue..Jan 5 2021, 6:18 AM
fhahn edited the summary of this revision. (Show Details)
Ayal added a comment.Jan 5 2021, 1:21 PM

(how) are the VPPredInstPHIRecipe changes related to the introduction of getLiveInIRValue()?

Patch is (NFC).

fhahn added a comment.Jan 5 2021, 2:10 PM

(how) are the VPPredInstPHIRecipe changes related to the introduction of getLiveInIRValue()?

Patch is (NFC).

They are not directly related, i didn’t realise they were already there before undoing the changes that were part of the original patch.

I can split them off and land them separately

Ayal accepted this revision.Jan 5 2021, 2:17 PM

ok, please land independent changes separately, thanks!

This revision is now accepted and ready to land.Jan 5 2021, 2:17 PM
fhahn updated this revision to Diff 314836.Jan 6 2021, 2:57 AM

Thanks Ayal! I landed the VPPredInstPHIRecipe changes separately in f73c09caa2a8.

This revision was landed with ongoing or failed builds.Jan 6 2021, 3:22 AM
This revision was automatically updated to reflect the committed changes.