[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 2 inline comments as done.
reames added a comment.

In D64215#1570640 <https://reviews.llvm.org/D64215#1570640>, @jdoerfert wrote:

> I like the idea. I browsed through the code but I failed to see where the poison is exposed to the user, e.g. through an assert or printf call. (Polly has a neat printf builder if that is of interest.)


At the moment, I have a placeholder trap(bool) signature used, but that's one of the areas which clearly needs work.



================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:1
+//===- PoisonChecking.cpp - -----------------------------------------------===//
+//
----------------
sanjoy wrote:
> Maybe just call this PoisonSanitizer?
I could easily be convinced, but since the existing convention is that sanatizers are compiler user tools, not compiler developer tools, I thought it might be better to pick a different name.   Weakly held opinion, happy to go with whatever reviewers prefer.


================
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
----------------
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.  



Repository:
  rL LLVM

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

https://reviews.llvm.org/D64215