Page MenuHomePhabricator

[NewPM] fixing asserts on deleted loop in -print-after-all
ClosedPublic

Authored by fedor.sergeev on Nov 19 2018, 11:14 PM.

Details

Summary

IR-printing AfterPass instrumentation might be called on a loop
that has just been invalidated. We should skip printing it to
avoid spurious asserts.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev created this revision.Nov 19 2018, 11:14 PM

oops... scc-deleted-printer.ll is not yet ready - skip it until updated.
everything else should be fine.

chandlerc added inline comments.Nov 19 2018, 11:22 PM
include/llvm/IR/PassInstrumentation.h
152–154 ↗(On Diff #174725)

I think this should have a different name to indicate that it is called in a fundamentally different situation -- after an IR unit is invalidated.

158–159 ↗(On Diff #174725)

Similarly, I think the callbacks should be different. The user can always do this binding with nullptr if that's a desirable way to model it, but generally I think different names of routines seem more appropriate.

lib/Passes/StandardInstrumentations.cpp
136–137 ↗(On Diff #174725)

FWIW, I think the way to handle this is to bind a reference to the module into the callback so that it intrinsically has access to it to print.

lib/Transforms/Scalar/LoopPassManager.cpp
47 ↗(On Diff #174725)

nit: Here and elsewhere in comments: please use proper prose with capitalized first letter and punctuation.

scc-deleted-printer test updated

fedor.sergeev added inline comments.Nov 19 2018, 11:53 PM
include/llvm/IR/PassInstrumentation.h
158–159 ↗(On Diff #174725)

Since this nullptr hack is not under control of the user its either the same callback with nullptr (as of right now) or a callback w/o any IR at all.

Conceptually, using different callbacks does look like a clearner solution, though I'm a bit worried that we double the amount of callbacks for a somewhat limited added value...
Need to try it out and see how it looks like.

lib/Passes/StandardInstrumentations.cpp
136–137 ↗(On Diff #174725)

Thats true, just wanted to do that separately (and, btw, address the same problem for Legacy printer).

moved "invalidated after-pass" into its own callback, looks like a proper solution, indeed.

adding new instrumentation point to unittests

philip.pfaffe added inline comments.Dec 3 2018, 4:43 AM
lib/Passes/StandardInstrumentations.cpp
136–137 ↗(On Diff #174725)

Same as above.

58 ↗(On Diff #174775)

Is that still the case? You don't hook up the Invalidated callback below.

fedor.sergeev marked an inline comment as done.Dec 3 2018, 5:56 AM
fedor.sergeev added inline comments.
lib/Passes/StandardInstrumentations.cpp
58 ↗(On Diff #174775)

Hmm... indeed. I did it in the followup which handles -print-module-scope part and it has not been posted yet :-(
Okey, let me clean this up once more.

chandlerc requested changes to this revision.Dec 4 2018, 2:07 AM

One place that I'm not seeing an update reflected (but I may just misunderstand)....

lib/Passes/StandardInstrumentations.cpp
58 ↗(On Diff #174775)

Did this happen?

This revision now requires changes to proceed.Dec 4 2018, 2:07 AM
fedor.sergeev marked an inline comment as done.Dec 4 2018, 6:19 AM
fedor.sergeev added inline comments.
lib/Passes/StandardInstrumentations.cpp
58 ↗(On Diff #174775)

not yet, lagging behind

slight cleanup in anticipation of a followup that handles invalidated printing

philip.pfaffe added inline comments.Dec 7 2018, 6:38 AM
lib/Passes/StandardInstrumentations.cpp
86 ↗(On Diff #176648)

Is there a policy on assertions?

Those you added imho don't add value. You're asserting for nullptr, but are using the pointer unconditionally after. That's failing just as hard. The message on the other hand isn't very helpful I feel, since you're not actually testing loop validity, but whether a nullptr was stored in the any, which I don't think is the same thing (anymore?).

fedor.sergeev marked an inline comment as done.Dec 7 2018, 9:08 AM
fedor.sergeev added inline comments.
lib/Passes/StandardInstrumentations.cpp
86 ↗(On Diff #176648)

Well... I have no idea about policies on assertions.

To me a very message of assert is usually considerably less helpful than the location of it.
Usually I dont read the message and just jump to the source when it hits.

You are right that now its not checking for validity, and it remains there only for the check against accidental nullptr (read - but in PassManager).
So yes, I might just scrap these...

removing unneeded asserts

Thanks, this does LGTM!

One additional thought though, and if this doesn't make sense to you feel free to commit:

In case the IRUnit has been invalidated, can we still pass it to the callback? When I think of instrumentations that do more complicated things than printing, it's highly likely that they would like to be notified when an IRUnit handle becomes invalid. Which requires passing the handle. If we are passing the handle, though, we need to emphasize that the unit isn't accessible through the handle anymore. Maybe by passing void* instead, or some tag type Invalidated<T> { void* Handle; }. The latter can go through the any and allow deducing the actual IRUnit type.

philip.pfaffe accepted this revision.Dec 10 2018, 7:04 AM

In case the IRUnit has been invalidated, can we still pass it to the callback? When I think of instrumentations that do more complicated things than printing, it's highly likely that they would like to be notified when an IRUnit handle becomes invalid. Which requires passing the handle. If we are passing the handle, though, we need to emphasize that the unit isn't accessible through the handle anymore. Maybe by passing void* instead, or some tag type Invalidated<T> { void* Handle; }. The latter can go through the any and allow deducing the actual IRUnit type.

That would require putting more order on what *is* the invalidated IRUnit.
Overall this looks like a fine solution that can handle come complicated cases, however I would rather follow a common approach and wait until the need for that complication arises.
Perhaps will add a comment somewhere in PassInstrumentation that hints about this.

The IRUnit is invalid, but the handle still exists, right?

Anyways, happy to do this as soon as this becomes an issue :)

The IRUnit is invalid, but the handle still exists, right?

Invalid handle might be totally invalid, you can not use it for anything.
I do remember having accesses to freed memory when accessing either Loop or SCC, dont remember which one.
Even looking at the address might be not that sane if it was freed and perhaps something else allocated at that place.

Anyway, my point is that we do not have a well established notion of invalid IR.
And solution that tries to put order into pass-instrumentation handling of invalid IR first
needs to put order into overall handling of that invalid IR.

The invalid handle isn't good for accessing anything, but it can still identify a unit.

The invalid handle isn't good for accessing anything, but it can still identify a unit.

Not really. If you say have a Loop* pointer, which is freed and then another Loop allocated happens to be just at that pointer
then you dont have a way to identify whether it is the old Loop or the new one.
Presumably that does not happen now...

Not really. If you say have a Loop* pointer, which is freed and then another Loop allocated happens to be just at that pointer
then you dont have a way to identify whether it is the old Loop or the new one.
Presumably that does not happen now...

The point is what kind of information you attach to the handle. When triggering the invalidation callbacks, the message is "This particular loop was invalidated". And that's a valid message, no matter if the handle is already being reused, because a downstream instrumentation cannot have observed the new loop behind the handle yet since we're still on the invalidation path.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 11 2018, 11:09 AM
This revision was automatically updated to reflect the committed changes.