This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Correct the made changes behavior
AbandonedPublic

Authored by bcl5980 on Feb 26 2023, 8:20 PM.

Details

Reviewers
nikic
fhahn
chill
Summary

For now in some code path, even if it can be removed we still set MadeChanges to true. This patch fix them to make sure the MadeChanges set to true only if it is really changed.

Diff Detail

Event Timeline

bcl5980 created this revision.Feb 26 2023, 8:20 PM
bcl5980 requested review of this revision.Feb 26 2023, 8:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2023, 8:20 PM
chill added inline comments.Feb 27 2023, 2:18 AM
llvm/lib/Transforms/Utils/SCCPSolver.cpp
238 ↗(On Diff #500652)

tryToReplaceWithConstant will return true if it made a change and we should set the flag regardless of whether we can remove the instruction.

bcl5980 updated this revision to Diff 500977.Feb 27 2023, 5:17 PM
nikic requested changes to this revision.Feb 28 2023, 1:03 AM
nikic added inline comments.
llvm/lib/Transforms/IPO/SCCP.cpp
215

Even in this case we're going to do a changeToUnreachable() below, so it's still changed?

This revision now requires changes to proceed.Feb 28 2023, 1:03 AM
bcl5980 abandoned this revision.Feb 28 2023, 2:25 AM
bcl5980 added inline comments.Feb 28 2023, 3:04 AM
llvm/lib/Transforms/IPO/SCCP.cpp
215

Oh thanks for the mention. You are right, generally it will change first non-phi instruction to unreachable.
How about if the entry block is already unreachable? It will replace unreachable to unreachable again. It looks this case we needn't set MadeChanges to true.
I have some local code to run ipsccp until no changed and dead loop happen here. This is the motivation of the patch. And I think I need to find some other way to workaround it maybe.