[PATCH] D112026: Treat branch on poison as immediate UB (under an off by default flag)

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: regehr, nikic, nlopes, lebedev.ri, hyeongyukim.
Herald added subscribers: javed.absar, bollu, hiraditya, mcrosier.
reames requested review of this revision.
Herald added a project: LLVM.

The LangRef clearly states that branching on a poison value is immediate undefined behavior, but historically, we have not been consistent about implementing that interpretation in the optimizer. Historically, we used (in some cases) a more relaxed model which essentially looked for provable UB along both paths which was control dependent on the condition. However, we've never been 100% consistent here.  For instance SCEV uses the strong model for increments which form AddRecs (and only addrecs).

At the moment, the last big blocker for finally making this switch is enabling the fix landed in D106041 <https://reviews.llvm.org/D106041>. Loop unswitching (in it's classic form) is incorrect as it creates many "branch on poisons" when unswitching conditions originally unreachable within the loop.

This change adds a flag to value tracking which allows to easily test the optimization potential of treating branch on poison as immediate UB.  It's intended to help ease work on getting us finally through this transition and avoid multiple independent rediscovers of the same issues.

D111246 <https://reviews.llvm.org/D111246> shows the test diff impact of enabling this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112026

Files:
  llvm/lib/Analysis/ValueTracking.cpp


Index: llvm/lib/Analysis/ValueTracking.cpp
===================================================================
--- llvm/lib/Analysis/ValueTracking.cpp
+++ llvm/lib/Analysis/ValueTracking.cpp
@@ -84,6 +84,17 @@
 static cl::opt<unsigned> DomConditionsMaxUses("dom-conditions-max-uses",
                                               cl::Hidden, cl::init(20));
 
+// According to the LangRef, branching on a poison condition is absolutely
+// immediate full UB.  However, historically we haven't implemented that
+// consistently as we have an important transformation (non-trivial unswitch)
+// which introduces instances of branch on poison/undef to otherwise well
+// defined programs.  This flag exists to let us test optimization benefit
+// of exploiting the specified behavior (in combination with enabling the
+// unswitch fix.)
+static cl::opt<bool> BranchOnPoisonAsUB("branch-on-poison-as-ub",
+                                        cl::Hidden, cl::init(false));
+
+
 /// Returns the bitwidth of the given scalar or pointer type. For vector types,
 /// returns the element type's bitwidth.
 static unsigned getBitWidth(Type *Ty, const DataLayout &DL) {
@@ -5466,7 +5477,16 @@
   case Instruction::SRem:
     Operands.insert(I->getOperand(1));
     break;
-
+  case Instruction::Switch:
+    if (BranchOnPoisonAsUB)
+      Operands.insert(cast<SwitchInst>(I)->getCondition());
+    break;
+  case Instruction::Br: {
+    auto *BR = cast<BranchInst>(I);
+    if (BranchOnPoisonAsUB && BR->isConditional())
+      Operands.insert(BR->getCondition());
+    break;
+  }
   default:
     break;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D112026.380492.patch
Type: text/x-patch
Size: 1603 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211018/37f19ae4/attachment.bin>