[JumpThreading] NFC: Add comment referencing #199408.

[jlebar]

Leave a breadcrumb indicating that not freeze'ing a select's condition
when turning it into a conditional branch is unsound (unless we can
prove that the cond is never poison). Nonetheless this doesn't seem to
cause real-world problems afaict.

[llvmorg-github-actions]

@llvm/pr-subscribers-llvm-transforms

Author: Justin Lebar (jlebar)

Changes

Leave a breadcrumb indicating that not freeze'ing a select's condition
when turning it into a conditional branch is unsound (unless we can
prove that the cond is never poison). Nonetheless this doesn't seem to
cause real-world problems afaict.

---

Full diff: https://github.com/llvm/llvm-project/pull/199718.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/JumpThreading.cpp (+7)

diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
index 415136b612ac2..602228bcb8289 100644
--- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp
@@ -2789,6 +2789,13 @@ void JumpThreadingPass::unfoldSelectInstr(BasicBlock *Pred, BasicBlock *BB,
   PredTerm->removeFromParent();
   PredTerm->insertInto(NewBB, NewBB->end());
   // Create a conditional branch and update PHI nodes.
+  //
+  // Note: Technically we should `freeze` the condition before using it in a
+  // conditional branch, unless we can prove it's not poison: select-on-on
+  // poison isn't UB, but branch-on-poison is.  But doing this causes
+  // performance regressions, and we haven't been able to find a source-level
+  // correctness issue it fixes.  See
+  // https://github.com/llvm/llvm-project/pull/199408#issuecomment-4545013881.
   auto *BI = CondBrInst::Create(SI->getCondition(), NewBB, BB, Pred);
   BI->applyMergedLocation(PredTerm->getDebugLoc(), SI->getDebugLoc());
   BI->copyMetadata(*SI, {LLVMContext::MD_prof});

[boomanaiden154]

LGTM, but it would be good for someone else to take a look too before merging.

[boomanaiden154]

s/select-on-on/select-on-poison?

[jlebar]

See that's what I get when I don't have an LLM write my code. :D

[antoniofrighetto]

I think there is a little bit of confusion. Regardless of whether this may not be arising from an end-to-end miscompilation issue, introducing a freeze is correct here (https://alive2.llvm.org/ce/z/cW2xqK). With select on poison, the select simply propagates poison through its result. Conversely, the semantics is not well-defined on which branch should be taken when branching on poison (more at: https://www.npopov.com/2022/12/20/This-year-in-LLVM-2022.html#branch-on-poison).

In general, when turning a select into a branch (despeculating), and the condition may be undef or poison, the freeze is needed to choose a non-deterministic, yet fixed value throughout the rest of the program. I put up #199736.

[boomanaiden154]

I think that was the understanding on the previous PR. A freeze here does make sense.

But a 0.1-0.2% performance regression is pretty big and it would be good to address that first.

[jlebar]

I think that was the understanding on the previous PR.

Yes, mine as well.