Hi folks,
Per the documentation[0], whenever an LLVM pass doesn't modify the IR
it's run on, it
should return `false`--it's okay to return `true` if no change happen, just
less
optimal. In the New PM area, this is generally translated into a
`PreservedAnalyses::all()`.
https://reviews.llvm.org/D80916 provides an `EXPENSIVE_CHECK` that computes a
hash of the IR before and after the pass, and checks that any change is
correctly reported. The hash is currently incomplete (on purpose, let's
start
small), but it turns out a dozen of passes do not satisfy that
requirement.
This could lead to various category of bugs (dangling references, inconsistent
state, etc). This affects both New and Legacy PM, as passes tend to wrap
functions
that report their status.
I wrote a bunch of patches for all failure detected by this check, as I cannot
land the
check now, it would break the buildbots :-) Any help to review the remaining
ones [1] is appreciated.
Once that check lands and we're relatively confident on the quality of the
return status, some more optimizations could be triggered, like
https://reviews.llvm.org/D80707.
[0] https://llvm.org/docs/WritingAnLLVMPass.html#the-runonmodule-method
[1] https://reviews.llvm.org/D81230
https://reviews.llvm.org/D81236
https://reviews.llvm.org/D81256
https://reviews.llvm.org/D81238
https://reviews.llvm.org/D81225
Hi, I think this is a very useful addition. Note that many passes using EXPENSIVE_CHECKS to verify themselves still allow switching verification on using a command line option (e.g. -verify-loop-info) where EXPENSIVE_CHECKS just controls the default value. This is useful so it can be enabled explicitly e.g. in regression tests for proper pass behavior. Michael Am Do., 11. Juni 2020 um 10:42 Uhr schrieb Serge Guelton via llvm-dev <llvm-dev at lists.llvm.org>:> > Hi folks, > > Per the documentation[0], whenever an LLVM pass doesn't modify the IR it's run on, it > should return `false`--it's okay to return `true` if no change happen, just less > optimal. In the New PM area, this is generally translated into a `PreservedAnalyses::all()`. > > https://reviews.llvm.org/D80916 provides an `EXPENSIVE_CHECK` that computes a > hash of the IR before and after the pass, and checks that any change is > correctly reported. The hash is currently incomplete (on purpose, let's start > small), but it turns out a dozen of passes do not satisfy that > requirement. > > This could lead to various category of bugs (dangling references, inconsistent > state, etc). This affects both New and Legacy PM, as passes tend to wrap functions > that report their status. > > I wrote a bunch of patches for all failure detected by this check, as I cannot land the > check now, it would break the buildbots :-) Any help to review the remaining > ones [1] is appreciated. > > Once that check lands and we're relatively confident on the quality of the > return status, some more optimizations could be triggered, like > https://reviews.llvm.org/D80707. > > > [0] https://llvm.org/docs/WritingAnLLVMPass.html#the-runonmodule-method > [1] https://reviews.llvm.org/D81230 > https://reviews.llvm.org/D81236 > https://reviews.llvm.org/D81256 > https://reviews.llvm.org/D81238 > https://reviews.llvm.org/D81225 > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> On Jun 11, 2020, at 8:42 AM, Serge Guelton via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > Hi folks, > > Per the documentation[0], whenever an LLVM pass doesn't modify the IR it's run on, it > should return `false`--it's okay to return `true` if no change happen, just less > optimal. In the New PM area, this is generally translated into a `PreservedAnalyses::all()`. > > https://reviews.llvm.org/D80916 provides an `EXPENSIVE_CHECK` that computes a > hash of the IR before and after the pass, and checks that any change is > correctly reported. The hash is currently incomplete (on purpose, let's start > small), but it turns out a dozen of passes do not satisfy that > requirement. > > This could lead to various category of bugs (dangling references, inconsistent > state, etc). This affects both New and Legacy PM, as passes tend to wrap functions > that report their status. > > I wrote a bunch of patches for all failure detected by this check, as I cannot land the > check now, it would break the buildbots :-) Any help to review the remaining > ones [1] is appreciated. > > Once that check lands and we're relatively confident on the quality of the > return status, some more optimizations could be triggered, like > https://reviews.llvm.org/D80707.Wow, this is really cool. Thank you for working on this! -Chris> > > [0] https://llvm.org/docs/WritingAnLLVMPass.html#the-runonmodule-method > [1] https://reviews.llvm.org/D81230 > https://reviews.llvm.org/D81236 > https://reviews.llvm.org/D81256 > https://reviews.llvm.org/D81238 > https://reviews.llvm.org/D81225 > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
On Thu, Jun 11, 2020 at 8:42 AM Serge Guelton via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hi folks, > > Per the documentation[0], whenever an LLVM pass doesn't modify the IR it's > run on, it > should return `false`--it's okay to return `true` if no change happen, > just less > optimal. In the New PM area, this is generally translated into a > `PreservedAnalyses::all()`. > > https://reviews.llvm.org/D80916 provides an `EXPENSIVE_CHECK` that > computes a > hash of the IR before and after the pass, and checks that any change is > correctly reported. The hash is currently incomplete (on purpose, let's > start > small), but it turns out a dozen of passes do not satisfy that > requirement. > > This could lead to various category of bugs (dangling references, > inconsistent > state, etc). This affects both New and Legacy PM, as passes tend to wrap > functions > that report their status. > > I wrote a bunch of patches for all failure detected by this check, as I > cannot land the > check now, it would break the buildbots :-) Any help to review the > remaining > ones [1] is appreciated. > > Once that check lands and we're relatively confident on the quality of the > return status, some more optimizations could be triggered, like > https://reviews.llvm.org/D80707. >Awesome feature! I am really fond of these pieces of infrastructure that can defend against human mistakes and save countless hours of debugging when subtle issues arise. Thanks Serge, -- Mehdi> > > [0] https://llvm.org/docs/WritingAnLLVMPass.html#the-runonmodule-method > [1] https://reviews.llvm.org/D81230 > https://reviews.llvm.org/D81236 > https://reviews.llvm.org/D81256 > https://reviews.llvm.org/D81238 > https://reviews.llvm.org/D81225 > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200612/ee7c0c0a/attachment.html>
Hi folks, some more information on this feature - as a reminder I started one month ago to work on an expensive check that would verify that pass return status is correctly reported by passes, i.e. no pass return « IR not modified » while actually modifying it. It took ~20 pass fixes to achieve that goal, as many passes were not respectful of that contract, but as of 3667d87a33d3c8d4072a41fd84bb880c59347dc0, https://reviews.llvm .org/D80916 has been merged in master and the check is active, which should prevent further regression on that topic. Thanks a lot to @foad, @jdoerfert, @fhahn, @calixte (and others I'm sorry to forgot) for their help during the reviews. On Fri, Jun 12, 2020 at 11:24 PM Mehdi AMINI <joker.eph at gmail.com> wrote:> > > On Thu, Jun 11, 2020 at 8:42 AM Serge Guelton via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Hi folks, >> >> Per the documentation[0], whenever an LLVM pass doesn't modify the IR >> it's run on, it >> should return `false`--it's okay to return `true` if no change happen, >> just less >> optimal. In the New PM area, this is generally translated into a >> `PreservedAnalyses::all()`. >> >> https://reviews.llvm.org/D80916 provides an `EXPENSIVE_CHECK` that >> computes a >> hash of the IR before and after the pass, and checks that any change is >> correctly reported. The hash is currently incomplete (on purpose, let's >> start >> small), but it turns out a dozen of passes do not satisfy that >> requirement. >> >> This could lead to various category of bugs (dangling references, >> inconsistent >> state, etc). This affects both New and Legacy PM, as passes tend to wrap >> functions >> that report their status. >> >> I wrote a bunch of patches for all failure detected by this check, as I >> cannot land the >> check now, it would break the buildbots :-) Any help to review the >> remaining >> ones [1] is appreciated. >> >> Once that check lands and we're relatively confident on the quality of the >> return status, some more optimizations could be triggered, like >> https://reviews.llvm.org/D80707. >> > > Awesome feature! I am really fond of these pieces of infrastructure that > can defend against human mistakes and save countless hours of debugging > when subtle issues arise. > > Thanks Serge, > > -- > Mehdi > > > >> >> >> [0] https://llvm.org/docs/WritingAnLLVMPass.html#the-runonmodule-method >> [1] https://reviews.llvm.org/D81230 >> https://reviews.llvm.org/D81236 >> https://reviews.llvm.org/D81256 >> https://reviews.llvm.org/D81238 >> https://reviews.llvm.org/D81225 >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200715/1dc0c3e7/attachment.html>