This is an archive of the discontinued LLVM Phabricator instance.

More conservatively report status from LoopIdiomRecognize
ClosedPublic

Authored by jroelofs on Jul 17 2020, 2:03 PM.

Details

Summary

Being "precise" here is getting us into trouble with one of the EXPENSIVE_CHECKS buildbots, see [1]. Rather than reporting IR additions that later get rolled back as "no change", instead we now conservatively report that there was.

1: http://lists.llvm.org/pipermail/llvm-dev/2020-July/143509.html

Diff Detail

Event Timeline

jroelofs created this revision.Jul 17 2020, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2020, 2:03 PM
nikic added inline comments.Jul 18 2020, 9:58 AM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
947

Pre-existing issue, but aren't we missing this for the early returns below? Probably this code should also use ExpandedValuesCleaner.

jroelofs marked 2 inline comments as done.Jul 20 2020, 7:52 AM
jroelofs added inline comments.
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
947

I'll take care of that in a follow-up commit: https://reviews.llvm.org/D84174

This revision is now accepted and ready to land.Jul 20 2020, 11:01 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Jul 23 2020, 7:50 AM

Do you happen to still have the test case for this? If it is reasonably small, would it be possible to add to the tests?

Do you happen to still have the test case for this? If it is reasonably small, would it be possible to add to the tests?

I have the IR, but the assert doesn't repro if I run it through the pass via opt. I'll see if I can reduce it a bit so we at least have a test where the PM reports the change.

Do you happen to still have the test case for this? If it is reasonably small, would it be possible to add to the tests?

I have the IR, but the assert doesn't repro if I run it through the pass via opt. I'll see if I can reduce it a bit so we at least have a test where the PM reports the change.

https://reviews.llvm.org/D84450