IR-printing AfterPass instrumentation might be called on a loop
that has just been invalidated. We should skip printing it to
avoid spurious asserts.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 25672 Build 25671: arc lint + arc unit
Event Timeline
oops... scc-deleted-printer.ll is not yet ready - skip it until updated.
everything else should be fine.
include/llvm/IR/PassInstrumentation.h | ||
---|---|---|
161–163 | 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. | |
167–168 | 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 | ||
130–131 | 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–51 | nit: Here and elsewhere in comments: please use proper prose with capitalized first letter and punctuation. |
include/llvm/IR/PassInstrumentation.h | ||
---|---|---|
167–168 | 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... | |
lib/Passes/StandardInstrumentations.cpp | ||
130–131 | 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.
lib/Passes/StandardInstrumentations.cpp | ||
---|---|---|
58 | Hmm... indeed. I did it in the followup which handles -print-module-scope part and it has not been posted yet :-( |
One place that I'm not seeing an update reflected (but I may just misunderstand)....
lib/Passes/StandardInstrumentations.cpp | ||
---|---|---|
58 | Did this happen? |
lib/Passes/StandardInstrumentations.cpp | ||
---|---|---|
58 | not yet, lagging behind |
lib/Passes/StandardInstrumentations.cpp | ||
---|---|---|
86 | 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?). |
lib/Passes/StandardInstrumentations.cpp | ||
---|---|---|
86 | 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. 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). |
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.
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 :)
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.
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.
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.