This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Tune cast instruction handling for overdefined operand
ClosedPublic

Authored by anton-afanasyev on Oct 28 2021, 7:39 AM.

Details

Summary

Extended value is known to be inside range smaller than full set.
Prevent SCCP to mark such value as overdefined.

Fixes PR52253

Diff Detail

Event Timeline

anton-afanasyev requested review of this revision.Oct 28 2021, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2021, 7:39 AM
fhahn added inline comments.Oct 28 2021, 9:48 AM
llvm/lib/Transforms/Utils/SCCPSolver.cpp
845

could that be handled by the code above, just with using the full range for OpRange if it is overdefined?

anton-afanasyev marked an inline comment as done.Oct 28 2021, 12:24 PM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Utils/SCCPSolver.cpp
845

Ok, changed it this way, though getting a few more redundant computations.

anton-afanasyev marked an inline comment as done.

Address comment

RKSimon resigned from this revision.Nov 3 2021, 6:18 AM

I'm sorry, I'm not familiar with this code

anton-afanasyev retitled this revision from [SCCP] Tune `zext` and `sext` processing for overdefined operand to [SCCP] Tune `zext` and `sext` handling for overdefined operand.Nov 8 2021, 1:54 AM
anton-afanasyev retitled this revision from [SCCP] Tune `zext` and `sext` handling for overdefined operand to [SCCP] Tune cast instruction handling for overdefined operand.
fhahn added a comment.Nov 8 2021, 4:18 AM

Thanks for the update! I think with the latest changes applied, I noticed another potential simplification.

llvm/lib/Transforms/Utils/SCCPSolver.cpp
821

IIUC we need to check for range or overdefined here explicitly because we implicitly do not do anything for under or overdefined.

Could we instead add an early bail out on undeforunknown, only check for integer types here (and not constant range and overdefined) and drop the markOverdefined in the else of below?

anton-afanasyev marked an inline comment as done.Nov 8 2021, 5:36 AM
anton-afanasyev added inline comments.
llvm/lib/Transforms/Utils/SCCPSolver.cpp
821

Sure, thanks, dit it that way. Potentially this could be not NFC and improve handling of NotConstant operand, but SCCP doesn't use NotConstant state, so actually no change at all.

anton-afanasyev marked an inline comment as done.

Address comment

fhahn accepted this revision.Nov 8 2021, 6:40 AM

LGTM, thanks!

llvm/lib/Transforms/Utils/SCCPSolver.cpp
821

Sure, thanks, dit it that way. Potentially this could be not NFC and improve handling of NotConstant operand, but SCCP doesn't use NotConstant state, so actually no change at all.

Yeah, NotConstant should not be used for integers at the very least, might be worth adding an assert here.

This revision is now accepted and ready to land.Nov 8 2021, 6:40 AM
This revision was landed with ongoing or failed builds.Nov 8 2021, 7:35 AM
This revision was automatically updated to reflect the committed changes.