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

[llvm] Add NCD search on Array of basic blocks (NFC) #119355

Merged

Conversation

enoskova-sc
Copy link
Contributor

@enoskova-sc enoskova-sc commented Dec 10, 2024

Shrink-Wrap points split Part 2.
RFC: https://discourse.llvm.org/t/shrink-wrap-save-restore-points-splitting/83581

Part 1: #117862
Part 3: #119357
Part 4: #119358
Part 5: #119359

@llvmbot
Copy link
Member

llvmbot commented Dec 10, 2024

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-backend-risc-v

Author: Elizaveta Noskova (enoskova-sc)

Changes

Shrink-Wrap points split Part 2.

Part 1: #117862 (comment)


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

3 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/MachineDominators.h (+5)
  • (modified) llvm/lib/CodeGen/MachineDominators.cpp (+16)
  • (modified) llvm/lib/Target/RISCV/RISCVFrameLowering.cpp (+4)
diff --git a/llvm/include/llvm/CodeGen/MachineDominators.h b/llvm/include/llvm/CodeGen/MachineDominators.h
index 74cf94398736dd..88800d91ef51a9 100644
--- a/llvm/include/llvm/CodeGen/MachineDominators.h
+++ b/llvm/include/llvm/CodeGen/MachineDominators.h
@@ -185,6 +185,11 @@ class MachineDominatorTree : public DomTreeBase<MachineBasicBlock> {
     return Base::findNearestCommonDominator(A, B);
   }
 
+  /// Returns the nearest common dominator of the given blocks.
+  /// If that tree node is a virtual root, a nullptr will be returned.
+  MachineBasicBlock *
+  findNearestCommonDominator(ArrayRef<MachineBasicBlock *> Blocks) const;
+
   MachineDomTreeNode *operator[](MachineBasicBlock *BB) const {
     applySplitCriticalEdges();
     return Base::getNode(BB);
diff --git a/llvm/lib/CodeGen/MachineDominators.cpp b/llvm/lib/CodeGen/MachineDominators.cpp
index a2cc8fdfa7c9f9..384f90c6da66c0 100644
--- a/llvm/lib/CodeGen/MachineDominators.cpp
+++ b/llvm/lib/CodeGen/MachineDominators.cpp
@@ -189,3 +189,19 @@ void MachineDominatorTree::applySplitCriticalEdges() const {
   NewBBs.clear();
   CriticalEdgesToSplit.clear();
 }
+
+MachineBasicBlock *MachineDominatorTree::findNearestCommonDominator(
+    ArrayRef<MachineBasicBlock *> Blocks) const {
+  assert(!Blocks.empty());
+
+  MachineBasicBlock *NCD = Blocks.front();
+  for (MachineBasicBlock *BB : Blocks.drop_front()) {
+    NCD = Base::findNearestCommonDominator(NCD, BB);
+
+    // Stop when the root is reached.
+    if (Base::isVirtualRoot(Base::getNode(NCD)))
+      return nullptr;
+  }
+
+  return NCD;
+}
diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
index deb0b627225c64..0de1f1d821a6e2 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -1607,6 +1607,8 @@ bool RISCVFrameLowering::assignCalleeSavedSpillSlots(
         int FrameIdx = MFI.CreateFixedSpillStackObject(Size, Offset);
         assert(FrameIdx < 0);
         CS.setFrameIdx(FrameIdx);
+        if (RISCVRegisterInfo::isRVVRegClass(RC))
+          MFI.setStackID(FrameIdx, TargetStackID::ScalableVector);
         continue;
       }
     }
@@ -1623,6 +1625,8 @@ bool RISCVFrameLowering::assignCalleeSavedSpillSlots(
     if ((unsigned)FrameIdx > MaxCSFrameIndex)
       MaxCSFrameIndex = FrameIdx;
     CS.setFrameIdx(FrameIdx);
+    if (RISCVRegisterInfo::isRVVRegClass(RC))
+      MFI.setStackID(FrameIdx, TargetStackID::ScalableVector);
   }
 
   // Allocate a fixed object that covers the full push or libcall size.

@enoskova-sc
Copy link
Contributor Author

@paperchalice, could you, please, review.


MachineBasicBlock *MachineDominatorTree::findNearestCommonDominator(
ArrayRef<MachineBasicBlock *> Blocks) const {
assert(!Blocks.empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

assert with error message if possible: https://llvm.org/docs/CodingStandards.html#id45

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

// Stop when the root is reached.
if (Base::isVirtualRoot(Base::getNode(NCD)))
return nullptr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Base::getNode here is unfortunate, use wrapped version here to get the latest dominator tree.
Addressed by #115111.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

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

Can you find a spot in existing code to use this factored out routine? A few ideas:
FindIDom in ShrinkWrap
DeleteUnreachable in GenericDomConstruction.
SILowerI1Copies.cpp
HexagonFrameLowering.cpp

Basically, there's a whole bunch of these. Please common a few so that we're not adding untested code.

This appears to be identical code to findNearestCommonDominator. Can we sink this into the GenericIDom layer instead?

@michaelmaitland
Copy link
Contributor

Reverse ping. Also, I think this could be rebased now that the base patch has landed.

@enoskova-sc enoskova-sc force-pushed the users/enoskova-sc/ncd-search-for-bbs-array branch from cecd892 to 1077335 Compare January 16, 2025 12:43
@enoskova-sc
Copy link
Contributor Author

@preames, @michaelmaitland, I have addressed your comments.

Copy link
Contributor

@michaelmaitland michaelmaitland left a comment

Choose a reason for hiding this comment

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

LGTM

I think you can mark the title as NFC?

@enoskova-sc enoskova-sc changed the title [llvm] Add NCD search on Array of basic blocks [llvm] Add NCD search on Array of basic blocks (NFC) Jan 17, 2025
@enoskova-sc
Copy link
Contributor Author

@preames, @paperchalice, any other comments?

@preames
Copy link
Collaborator

preames commented Jan 21, 2025

@preames, @paperchalice, any other comments?

You've got an LGTM and we're a one approval culture. My prior comments about reuse still stands, but that can easily be extended in a follow up commit.

@asi-sc asi-sc merged commit 3088c31 into llvm:main Jan 22, 2025
9 checks passed
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.

6 participants