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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

I think Alive and this are complementary.  Alive is useful in proving the correctness of _new_ transformations, whereas this pass should let us quickly figure out if a large unreduced application is misbehaving it branches on poison.



================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:91
+
+static Value *buildOrChain(IRBuilder<> &B, ArrayRef<Value*> Ops) {
+  if (Ops.size() == 0)
----------------
This looks like it can live on `IRBuilder`.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:95
+  unsigned i = 0;
+  for (; i < Ops.size() && isConstantFalse(Ops[i]); i++) {}
+  if (i == Ops.size())
----------------
Is all of this necessary?  I'd expect one round of instsimplify to fix up this kind of stuff.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:166
+static Value *getPoisonFor(DenseMap<Value *, Value *> &ValToPoison, Value *V) {
+  if (ValToPoison.count(V))
+    return ValToPoison[V];
----------------
You can do a single iterator lookup here instead of two.


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:181
+  if (auto *CI = dyn_cast<ConstantInt>(Cond))
+    if (CI->isZero())
+      return;
----------------
It seems like the trap function assert that the condition is false?  If yes then it should probably be called something else I think (something like `assert_is_false` would be great).


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:226
+      if (propagatesFullPoison(&I))
+        for (Use &U : I.operands())
+          Checks.push_back(getPoisonFor(ValToPoison, U.get()));
----------------
Can't you directly do `for (Value *V: I.operands())`?


================
Comment at: lib/Transforms/Instrumentation/PoisonChecking.cpp:267
+
+  PreservedAnalyses PA;
+  return PA;
----------------
There is a `PreservedAnalyses::none()`.


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

https://reviews.llvm.org/D64215