Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CodeMoverUtils] Enhance CodeMoverUtils to sink an entire BB #87857

Conversation

CongzheUalberta
Copy link
Contributor

@CongzheUalberta CongzheUalberta commented Apr 6, 2024

When moving an entire basic block after InsertPoint, currently we check each instruction whether their users are dominated by InsertPoint, however, this can be improved such that even a user is not dominated InsertPoint, as long as it appears as a subsequent instruction in the same BB, it is safe to move.

This patch is similar to commit 751be2a that enhanced hoisting an entire BB, and this patch enhances sinking an entire BB. Please refer to the added functionality in test case llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp that was not supported without this patch.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 6, 2024

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Congzhe (CongzheUalberta)

Changes

When moving an entire basic block after InsertPoint, currently we check each instruction whether their users are dominated by InsertPoint, however, this can be improved such that even a user is not dominated InsertPoint, as long as it appears as a subsequent instruction in the same BB, it is safe to move.

This patch is similar to commit 751be2a that enhanced hoisting an entire BB, and this patch enhances sinking an entire BB.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/CodeMoverUtils.cpp (+10-2)
  • (modified) llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp (+8)
diff --git a/llvm/lib/Transforms/Utils/CodeMoverUtils.cpp b/llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
index 6a2dae5bab68ee..8b9526ceda9a3a 100644
--- a/llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
+++ b/llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
@@ -337,8 +337,16 @@ bool llvm::isSafeToMoveBefore(Instruction &I, Instruction &InsertPoint,
   if (isReachedBefore(&I, &InsertPoint, &DT, PDT))
     for (const Use &U : I.uses())
       if (auto *UserInst = dyn_cast<Instruction>(U.getUser()))
-        if (UserInst != &InsertPoint && !DT.dominates(&InsertPoint, U))
-          return false;
+        if (UserInst != &InsertPoint && !DT.dominates(&InsertPoint, U)) {
+          // If UserInst is an instruction that appears later in the same BB as
+          // I, then it is okay to move since I will still be available when
+          // UserInst is executed.
+          if (CheckForEntireBlock && I.getParent() == UserInst->getParent() &&
+              DT.dominates(&I, UserInst))
+            continue;
+          if (UserInst != &InsertPoint && !DT.dominates(&InsertPoint, U))
+            return false;
+        }
   if (isReachedBefore(&InsertPoint, &I, &DT, PDT))
     for (const Value *Op : I.operands())
       if (auto *OpInst = dyn_cast<Instruction>(Op)) {
diff --git a/llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp b/llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
index 8554d1a33cadee..e599450f25e0ee 100644
--- a/llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
+++ b/llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
@@ -615,6 +615,7 @@ TEST(CodeMoverUtils, IsSafeToMoveTest3) {
         Instruction *CmpInst = getInstructionByName(F, "cmp");
         BasicBlock *BB0 = getBasicBlockByName(F, "for.body");
         BasicBlock *BB1 = getBasicBlockByName(F, "for.latch");
+        BasicBlock *BB2 = getBasicBlockByName(F, "for.end");
 
         // Can move as the incoming block of %inc for %i (%for.latch) dominated
         // by %cmp.
@@ -625,6 +626,13 @@ TEST(CodeMoverUtils, IsSafeToMoveTest3) {
         // before %add2 although %add does not dominate InsertPoint.
         EXPECT_TRUE(
             isSafeToMoveBefore(*BB1, *BB0->getTerminator(), DT, &PDT, &DI));
+
+        // Can move as the operands of instructions in BB1 either dominate
+        // InsertPoint or appear before that instruction, e.g., %add appears
+        // before %add2 although %add does not dominate InsertPoint.
+        EXPECT_TRUE(
+            isSafeToMoveBefore(*BB1, *BB2->getTerminator(), DT, &PDT, &DI));
+
       });
 }
 

@CongzheUalberta CongzheUalberta force-pushed the 0001-CodeMoverUtils-Enhance-CodeMoverUtils-to-sink-entire branch 3 times, most recently from f79595e to 3924918 Compare April 7, 2024 04:30
Copy link

github-actions bot commented Apr 7, 2024

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

When moving an entire basic block BB after InsertPoint, currently
we check for all instructions whether their users are dominated by
InsertPoint, however, this can be improved such that even a user
is not dominated InsertPoint, as long as it appears as a subsequent
instruction in the same BB, it is safe to move.

This patch is similar to commit 751be2a
that enhanced hoisting an entire BB, and this patch enhances sinking an
entire BB.
@CongzheUalberta CongzheUalberta force-pushed the 0001-CodeMoverUtils-Enhance-CodeMoverUtils-to-sink-entire branch from 3924918 to 96ce0b2 Compare April 8, 2024 18:14
@CongzheUalberta CongzheUalberta merged commit b0662a7 into llvm:main Apr 10, 2024
4 checks passed
@CongzheUalberta
Copy link
Contributor Author

Thank you Whitney for the review! I very much appreciate it.
I've now merged this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants