[PATCH] D64215: Add a transform pass to make the executable semantics of poison explicit in the IR

[Philip Reames via Phabricator via llvm-commits]

reames marked 3 inline comments as done.
reames added inline comments.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:41
+//   are well defined on the specific input used.
+// - Finding/confirming poison specific miscompiles by checking the poison
+//   status of an input/IR pair is the same before and after an optimization
----------------
reames wrote:
> sanjoy wrote:
> > This will have false positives as long as `undef` is a thing right?  Unless the instrumentation added by this pass will produce poison if *any* value of `undef` produces poison (naively it seems this would require calling into a SAT solver at runtime)?
> I don't actually follow what you're trying to say.  Can you maybe rephrase w/an example which concerns you?  
> 
> For context, I'm focusing on poison for the moment, but I think a completely reasonable thing would be to have a parallel approach for undef (1 bit per bit of original value), and then cross propagate if needed.  I have a much weaker understanding of the undef rules, so I started with poison since that seemed a bit more clearly specified and recently discussed.  
> 
For context, the part which has me confused was your claim of a false positive.  I definitely see how undef can cause a false negative if not accounted for in the poison tracking.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:105
+    return buildOrChain(B, Checks);
+  }
+
----------------
aqjune wrote:
> This is great.
> Is it assumed that poison is always value-wise?
> In other words, if the result is 1, does it mean either:
> (1) the value is always fully poison
> (2) the value has at least one poison 'bit'?
> 
> In the future, there might exist several kinds of poison semantics that people would like to test (e.g. bitwise poison vs. valuewise poison) - so I think it is a good idea to describe which poison semantics this visitAdd is assuming.
All of this is assuming a single poison bit since that is our current semantics.  I'll leave it up to anyone wishing to evaluate alternate semantics to implement them.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:124
+    return buildOrChain(B, Checks);
+  }
+};
----------------
aqjune wrote:
> How about merging the contents of `visitSub` and `visitAdd` as they are equivalent except the intrinsics name? It can be reused for `visitMul` as well.
I agree the code structure needs some work here.  I'm experimenting with variations on that to see what looks least ugly and will upload a revised patch later tonight or tomorrow.  


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64215/new/

https://reviews.llvm.org/D64215