[NFC][Sink] Change runtime checks to asserts

[LU-JOHN]

Candidate block for sinking must be dominated by current location. This is true based on how the candidate block was selected. Runtime checks are not necessary and has been changed to an assertion.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (LU-JOHN)

Changes

Candidate block for sinking must be dominated by current location. This is true based on how the candidate block was selected. A runtime checks are not necessary and has been changed to an assertion.

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/Sink.cpp (+9-7)

diff --git a/llvm/lib/Transforms/Scalar/Sink.cpp b/llvm/lib/Transforms/Scalar/Sink.cpp
index 1a48a59c4189e..d6fd485aa5374 100644
--- a/llvm/lib/Transforms/Scalar/Sink.cpp
+++ b/llvm/lib/Transforms/Scalar/Sink.cpp
@@ -82,10 +82,11 @@ static bool IsAcceptableTarget(Instruction *Inst, BasicBlock *SuccToSinkTo,
         !Inst->hasMetadata(LLVMContext::MD_invariant_load))
       return false;
 
-    // We don't want to sink across a critical edge if we don't dominate the
-    // successor. We could be introducing calculations to new code paths.
-    if (!DT.dominates(Inst->getParent(), SuccToSinkTo))
-      return false;
+    // The current location of Inst dominates all uses, thus it must dominate
+    // SuccToSinkTo, which is on the IDom chain between the nearest common
+    // dominator to all uses and the current location.
+    assert(DT.dominates(Inst->getParent(), SuccToSinkTo) &&
+           "SuccToSinkTo must be dominated by current Inst location!");
 
     // Don't sink instructions into a loop.
     Loop *succ = LI.getLoopFor(SuccToSinkTo);
@@ -144,9 +145,10 @@ static bool SinkInstruction(Instruction *Inst,
       SuccToSinkTo = DT.findNearestCommonDominator(SuccToSinkTo, UseBlock);
     else
       SuccToSinkTo = UseBlock;
-    // The current basic block needs to dominate the candidate.
-    if (!DT.dominates(BB, SuccToSinkTo))
-      return false;
+    // The current basic block dominates all uses, thus it must dominate
+    // SuccToSinkTo, the nearest common dominator of all uses.
+    assert(DT.dominates(BB, SuccToSinkTo) &&
+           "SuccToSinkTo must be dominated by current basic block!");
   }
 
   if (SuccToSinkTo) {

[shiltian]

This is not an NFC when assert is disabled.

[nikic]

Nice catch, looks like this is a leftover from an old implementation prior to f2284e3.

[nikic]

I think it would be better to replace the two asserts with a single one at the end of this function.

[LU-JOHN]

I've replaced the two asserts with one assert, but I've put the assert at the beginning of the function since it is already true at that point.

[nikic]

The reason why I suggest to put it at the end is that SuccToSinkTo is changed in the function. So we are asserting that the property still holds after these adjustments.

[LU-JOHN]

Moved assert to when SuccToSinkTo is used to sink instruction.

[LU-JOHN]

This is not an NFC when assert is disabled.

When assert is disabled I've removed an if-block where the condition can never be true. Isn't this a NFC?

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[nikic]

LGTM

[shiltian]

This is not an NFC when assert is disabled.

When assert is disabled I've removed an if-block where the condition can never be true. Isn't this a NFC?

I was thinking that if there's any chance the condition in the assertion doesn't hold, and the assertion is also removed, then without this PR it would bail out, but now it would simply continue. So it’s not actually an NFC.