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

[GlobalISel] Allow some load/store instructions to be folded in Match Table backend #70830

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dzhidzhoev
Copy link
Member

Load/store instruction can be folded into non-adjacent instruction
within the same basic block, if there are no other instructions with
memory/other side effects between them.

… Table backend

Load/store instruction can be folded into non-adjacent instruction
within the same basic block, if there are no other instructions with
memory/other side effects between them.
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 31, 2023

@llvm/pr-subscribers-llvm-globalisel

Author: Vladislav Dzhidzhoev (dzhidzhoev)

Changes

Load/store instruction can be folded into non-adjacent instruction
within the same basic block, if there are no other instructions with
memory/other side effects between them.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp (+21-2)
diff --git a/llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp b/llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp
index 26752369a7711a1..3441df8a342e4f1 100644
--- a/llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/GIMatchTableExecutor.cpp
@@ -70,6 +70,25 @@ bool GIMatchTableExecutor::isObviouslySafeToFold(MachineInstr &MI,
   if (MI.isConvergent() && MI.getParent() != IntoMI.getParent())
     return false;
 
-  return !MI.mayLoadOrStore() && !MI.mayRaiseFPException() &&
-         !MI.hasUnmodeledSideEffects() && MI.implicit_operands().empty();
+  auto IsSafe = [](const MachineInstr &MI) {
+    return !MI.mayRaiseFPException() && !MI.hasUnmodeledSideEffects() &&
+           MI.implicit_operands().empty();
+  };
+  auto IsSafeNoMem = [IsSafe](const MachineInstr &MI) {
+    return !MI.mayLoadOrStore() && IsSafe(MI);
+  };
+
+  // If source instruction uses memory, fold if no intermediate
+  // instructions use it.
+  if (MI.mayLoadOrStore() && IsSafe(MI) &&
+      MI.getParent() == IntoMI.getParent()) {
+    auto IntoIt = IntoMI.getIterator();
+    auto NextIt = std::next(MI.getIterator());
+    while (!NextIt.isEnd() && NextIt != IntoIt && IsSafeNoMem(*NextIt))
+      ++NextIt;
+    if (NextIt == IntoIt)
+      return true;
+  }
+
+  return IsSafeNoMem(MI);
 }

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests

!MI.hasUnmodeledSideEffects() && MI.implicit_operands().empty();
auto IsSafe = [](const MachineInstr &MI) {
return !MI.mayRaiseFPException() && !MI.hasUnmodeledSideEffects() &&
MI.implicit_operands().empty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand the implicit operands check. I think this is starting to reproduce MachineInstr::isSafeToMove

@arsenm
Copy link
Contributor

arsenm commented Jan 30, 2024

reverse ping?

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be reinventing isSafeToMove

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