Page MenuHomePhabricator

[ValueTracking] emit a warning when we detect a conflicting assumption (PR31809)
ClosedPublic

Authored by spatel on Feb 1 2017, 10:21 AM.

Details

Summary

This is a follow-up to D29395 where we try to be good citizens and let the user know that we've gone off the rails.
As discussed previously, I'm also removing the comment about invalidating the assumption cache because that's unlikely to help and just adds more code where we don't want it.

This should allow us to resolve:
https://llvm.org/bugs/show_bug.cgi?id=31809

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Feb 1 2017, 10:21 AM
davide added a subscriber: anemet.Feb 1 2017, 11:45 AM
davide added inline comments.
lib/Analysis/ValueTracking.cpp
800–804 ↗(On Diff #86667)

I'm struggling here. It's true we're emitting a diagnostic related to the fact we can't optimize, but it's also true we're not optimizing because the IR to which we lowered exhibits undefined behaviour (so, there's a correctness issue).
Also, I thought we could use OptimizationRemarkEmitter for this kind of tasks? (+ Adam) @anemet

test/Transforms/InstSimplify/assume.ll
4–7 ↗(On Diff #86667)

Can we actually get a test to make sure we emit the correct debuginfo/debugloc instead of <unknown> ?

davide requested changes to this revision.Feb 1 2017, 11:45 AM
This revision now requires changes to proceed.Feb 1 2017, 11:45 AM
anemet added inline comments.Feb 1 2017, 12:27 PM
lib/Analysis/ValueTracking.cpp
800–804 ↗(On Diff #86667)

I am not sure this should be an optimization failure. I think (@hfinkel, correct me if I am wrong), we use failure for cases where the user has explicitly asked us to optimization the code in a way that we fail to do. I.e. it's a pretty strong diagnostics -- almost like a warning.

In this case I am not sure how severe this message should be. We can emit a missed optimization remark or an analysis remark, the latter being the catch-all and usually the most noisy.

In terms of the API used, this is the legacy API that we're trying to move away from. It would be great to convert instsimplify to require OptimizationRemarkEmitter analysis and then stash that somewhere (in Q perhaps?!) and then use the new ORE->emit(remark) interface.

spatel updated this revision to Diff 86742.Feb 1 2017, 3:56 PM
spatel edited edge metadata.

Patch updated:

  1. Use newer OptimizationRemarkEmitter API (old API is not marked deprecated?)
  2. Use OptimizationRemarkMissed().
  3. Updated tests to include debug info so it appears in the CHECK lines.
  4. Removed independent changes about invalidating the cache (rL293823).
davide requested changes to this revision.Feb 1 2017, 4:03 PM

I think this patch has still some problems (but please correct me if I'm wrong).
Here you're adding a dependency on OptimizationRemarkEmitter but, isn't that supposed to be used as an analysis?
If so, your pass should probably request it (and you should stick the ORE somewhere).

This revision now requires changes to proceed.Feb 1 2017, 4:03 PM
spatel added inline comments.Feb 1 2017, 4:09 PM
lib/Analysis/ValueTracking.cpp
800–804 ↗(On Diff #86667)

I've updated as suggested, but this doesn't seem right to me (I have never touched this API before this, so it's likely I haven't used this as intended...).

When we hit this case, we're headed for disaster: either we have a bug in LLVM or the program has UB. Either way, it's unlikely that the customer is going to be happy with the output. Is "OptimizationRemarkMissed" strong enough? I thought something equivalent to a stern warning would be appreciated here.

This API requires a pass name, but this is just value tracking, so we don't know which pass is actually getting us into here?

davide added inline comments.Feb 1 2017, 4:13 PM
lib/Analysis/ValueTracking.cpp
800–804 ↗(On Diff #86667)

I think what Adam is actually suggesting (or well, at least what I'm actually suggesting) is to make InstCombine/InstSimplify dependent on the OptimizationRemarksEmittedAnalysis (assuming we really want to go this route, it may make sense or not).

anemet added inline comments.Feb 1 2017, 4:32 PM
lib/Analysis/ValueTracking.cpp
800–804 ↗(On Diff #86667)

When we hit this case, we're headed for disaster: either we have a bug in LLVM or the program has UB. Either way, it's unlikely that the customer is going to be happy with the output. Is "OptimizationRemarkMissed" strong enough? I thought something equivalent to a stern warning would be appreciated here.

OK, so go back to failure. Sorry, I haven't been following the discussion. Failure is I think surfaced as a backend warning. You might want to check. The vectorizer emits those for loops where vectorization was requested by a pragma but we couldn't do it.

This API requires a pass name, but this is just value tracking, so we don't know which pass is actually getting us into here?

I don't think that using value-tracking would cause any issues for now. On the other hand the second parameter is an identifier and we use camel-cased single word.

hfinkel added inline comments.Feb 1 2017, 7:52 PM
lib/Analysis/ValueTracking.cpp
800–804 ↗(On Diff #86667)

OK, so go back to failure. Sorry, I haven't been following the discussion. Failure is I think surfaced as a backend warning. You might want to check. The vectorizer emits those for loops where vectorization was requested by a pragma but we couldn't do it.

I agree.

anemet added inline comments.Feb 1 2017, 9:55 PM
lib/Analysis/ValueTracking.cpp
800–804 ↗(On Diff #86667)

Turns out I never ported the vectorizer to use OptimizationFailure via ORE. rL293866 fixes this omission.

spatel updated this revision to Diff 86837.Feb 2 2017, 9:53 AM
spatel edited edge metadata.

Patch updated:
Use DiagnosticInfoOptimizationFailure again because we do want a warning here.

But this isn't behaving the way I expected: shouldn't this show up as a "warning" rather than a "remark" and without needing to specify "-pass-remarks-missed=.*"? Ie, DS_Warning was specified in the creation of the underlying DiagnosticInfoIROptimization().

Sorry if I'm still mis-using the APIs...

anemet requested changes to this revision.Feb 2 2017, 10:38 AM

But this isn't behaving the way I expected: shouldn't this show up as a "warning" rather than a "remark" and without needing to specify "-pass-remarks-missed=.*"? Ie, DS_Warning was specified in the creation of the underlying DiagnosticInfoIROptimization().

Yes, it should:

$ ./bin/opt -loop-vectorize ../test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll -o /dev/null
remark: source.cpp:19:5: loop not vectorized: cannot identify array bounds
warning: source.cpp:19:5: loop not vectorized: failed explicitly specified loop vectorization

The remark part is special-cased in LV to always appear but I think the warning is supposed to show up regardless.

Also once you resolve this part, you still need to change the patch to use ORE as an analysis pass via InstSimplify.

This revision now requires changes to proceed.Feb 2 2017, 10:38 AM

When we hit this case, we're headed for disaster: either we have a bug in LLVM or the program has UB.

"the program has UB" is a bit ambiguous. It's possible the "llvm.assume" call is dynamically unreachable, and therefore the undefined behavior doesn't matter. The dynamically unreachable codepath might not even exist in the original source code; for example, loop unswitching could duplicate a loop, and then one of "llvm.assume" calls becomes unreachable due to a correlated condition. This is one of the reasons LLVM doesn't emit warnings when the optimizer discovers code with undefined behavior.

When we hit this case, we're headed for disaster: either we have a bug in LLVM or the program has UB.

"the program has UB" is a bit ambiguous. It's possible the "llvm.assume" call is dynamically unreachable, and therefore the undefined behavior doesn't matter. The dynamically unreachable codepath might not even exist in the original source code; for example, loop unswitching could duplicate a loop, and then one of "llvm.assume" calls becomes unreachable due to a correlated condition. This is one of the reasons LLVM doesn't emit warnings when the optimizer discovers code with undefined behavior.

Ah, I didn't think of that possibility. In that case, abandon patch?

spatel updated this revision to Diff 86974.Feb 3 2017, 9:11 AM
spatel edited edge metadata.

Patch updated:

  1. Given Eli's observation about the potential to hit this case validly, soften this back to a OptimizationRemarkAnalysis() and use wishy-washy words in the remark.
  2. Add SimplifyInstructions dependency on OptimizationRemarkEmitter and stash the ORE pointer in the Query struct as suggested. I've tried to minimize the diffs by making the ORE an optional param, but this still seems to require a lot of code. If I understand correctly, we would have to do this for all passes that use value tracking and update all calls into value tracking to guarantee that a remark is printed.
anemet accepted this revision.Feb 3 2017, 9:51 AM

This LGTM now both in terms of the usage of ORE and the nature of the diagnostic. It would be good to have someone else also sign off on the choice of the diagnostics though.

lib/Analysis/ValueTracking.cpp
80 ↗(On Diff #86974)

Say that this could really be null (unlike other analyses) because not all clients provide it currently.

It seems fine to emit an optimization remark here. I don't recall the details about the different levels of optimization remarks off the top of my head, though.

This revision was automatically updated to reflect the committed changes.