[PATCH] D29404: [ValueTracking] emit a warning when we detect a conflicting assumption (PR31809)

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Analysis/ValueTracking.cpp:800-804
+
+    Q.CxtI->getContext().diagnose(DiagnosticInfoOptimizationFailure(
+        *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).


https://reviews.llvm.org/D29404