[PATCH] D29404: [ValueTracking] emit a warning when we detect a conflicting assumption (PR31809)
Adam Nemet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 1 16:32:10 PST 2017
anemet added inline comments.
Comment at: lib/Analysis/ValueTracking.cpp:800-804
+ *Q.CxtI->getFunction(), Q.CxtI->getDebugLoc(),
+ "Detected conflicting code assumptions. Program has undefined behavior "
+ "or internal compiler error."));
> spatel wrote:
> > anemet wrote:
> > > davide wrote:
> > > > 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
> > > 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.
> > >
> > 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?
> 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).
> 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.
More information about the llvm-commits