Page MenuHomePhabricator

Return "[NFC] Add severe validation of InstructionPrecedenceTracking"
ClosedPublic

Authored by mkazantsev on Aug 30 2018, 5:07 PM.

Details

Summary

This validation patch has been reverted as rL341147 because of conserns raised by
@reames. This revision returns it as is to raise a discussion and address the concerns.

Diff Detail

Repository
rL LLVM

Event Timeline

reames requested changes to this revision.Aug 30 2018, 9:38 PM
reames added inline comments.
include/llvm/Analysis/InstructionPrecedenceTracking.h
51 ↗(On Diff #163451)

I really really think there should be a destructor here which checks to make sure the information is still valid. If the info is no longer used, a client can call clear explicitly.

lib/Analysis/InstructionPrecedenceTracking.cpp
28 ↗(On Diff #163451)

As noted, calling this here makes this operation unexpected O(Inst in F) in the worst case.

Please check only the containing block.

67 ↗(On Diff #163451)

This comment caused me to misread. Something more along the lines of:
// For each block we have cached, make sure we cached the right answer

This revision now requires changes to proceed.Aug 30 2018, 9:38 PM
mkazantsev added inline comments.Sep 2 2018, 8:36 PM
include/llvm/Analysis/InstructionPrecedenceTracking.h
51 ↗(On Diff #163451)

I don't understand why we want it to be valid on destructor. We definitely are not going to use this after the destructor is executed, it's OK if it contains garbage. We only want the contents to be OK on *queries*.

mkazantsev added inline comments.Sep 2 2018, 8:41 PM
lib/Analysis/InstructionPrecedenceTracking.cpp
28 ↗(On Diff #163451)

This is intentional. If we have modified a block and haven't dropped cached info, we want to detect it as early as possible, specifically when we make *any* query to this tracking. Imagine we've screwed up invalidating a block A, then did 100 correct modifications and queries to other blocks and then requested something for A. The assertion will fail, and it will be very hard to figure out what and when has happened.

On the other hand, in current strategy we will be able to detect that we've screwed up whenever any query is made, which was the entire point of making this patch.

I don't think that win on compile time in debug mode is worth making investigation of functional bugs more complex.

mkazantsev updated this revision to Diff 163969.Sep 4 2018, 9:20 PM

After offline discussion with Philip, we agreed on the following version: by default, we have one-block verification to keep compile time in reasonable bounds. For troubleshooting purposes, there is an option (false by default and with no intention to turn it on) to diagnose it. Assertion on destructor does not happen because we don't have a commitment to keep the information valid after the last point where the last query was made.

LGTM w/ changes made before submit.

include/llvm/Analysis/InstructionPrecedenceTracking.h
42 ↗(On Diff #163969)

You're better off defining the behaviour as being erroneous. (i.e. illegal). Suggest alternate wording:
This helps to catch the usage error of accessing a block without properly invalidating after a previous transform.

lib/Analysis/InstructionPrecedenceTracking.cpp
77 ↗(On Diff #163969)

Optional, can be follow up: You'd be better to reduce the scope of the conditional compilation by using a conditional early return in this function.

101 ↗(On Diff #163969)

You can write this as:
assert(KnownBlocks.size() == FirstSpecialInsts.size())

since you've already validated each bloc, if there are no other special instructions, no further validation is needed.

mkazantsev added inline comments.Sep 5 2018, 8:42 PM
lib/Analysis/InstructionPrecedenceTracking.cpp
101 ↗(On Diff #163969)

It is not true. We can have some block in KnownBlocks and nothing in FirstSpecialInsts, and it will mean that there is no special instructions in this block. Maybe it makes sense to put nullptr for such blocks instead and completely eliminate KnownBlocks, I'll do it as a follow-up NFC.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 6 2018, 1:34 AM
This revision was automatically updated to reflect the committed changes.
mkazantsev added inline comments.Sep 6 2018, 2:38 AM
lib/Analysis/InstructionPrecedenceTracking.cpp
101 ↗(On Diff #163969)

https://reviews.llvm.org/rL341531

This looks much more clearly now.