Page MenuHomePhabricator

[SCEV] Turn validity check in getExistingSCEV into assert (NFC).
ClosedPublic

Authored by fhahn on Nov 26 2021, 4:37 AM.

Details

Summary

Now that we track users of SCEV expressions, we should be able to always
invalidate containing expressions.

With that, I think the case where a value gets removed but
SCEVs containing references to it should not be possible any longer.
Turn check into an assert.

This slightly reduces compile-time:

NewPM-O3: -0.27%
NewPM-ReleaseThinLTO: -0.21%
NewPM-ReleaseLTO-g: -0.26%

http://llvm-compile-time-tracker.com/compare.php?from=c3dc6b081da6ba503e67d260033f81f61eb38ea3&to=95a4a028b1f1dd0bc3d221435953b7d2c031b3d5&stat=instructions

Diff Detail

Event Timeline

fhahn created this revision.Nov 26 2021, 4:37 AM
fhahn requested review of this revision.Nov 26 2021, 4:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2021, 4:37 AM
nikic requested changes to this revision.Nov 26 2021, 4:51 AM

Duplicate of D113349/D114260.

This revision now requires changes to proceed.Nov 26 2021, 4:51 AM
nikic accepted this revision.Nov 26 2021, 12:00 PM

LGTM, now that D113349 is relanded. I'll abandon D114260 as the request there was to keep the assert, which is exactly what you did here.

This revision is now accepted and ready to land.Nov 26 2021, 12:00 PM
fhahn added a comment.Nov 27 2021, 5:37 AM

LGTM, now that D113349 is relanded. I'll abandon D114260 as the request there was to keep the assert, which is exactly what you did here.

Sounds good & thanks for linking the related patches, I missed them!

If I understand @mkazantsev's comment for D114260 correctly, we could still remove the check after the assert has been active for a while, to catch any potential offenders. So perhaps we could land D114260 in a month or 2?