[Xtensa] Implement support for the BranchRelaxation. #113450

andreisfr · 2024-10-23T12:38:16Z

No description provided.

github-actions · 2024-10-23T12:41:42Z

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

arsenm · 2024-10-25T23:21:07Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+                                            const XtensaInstrInfo &TII) {
+  unsigned FnSize = 0;
+  for (auto &MBB : MF) {
+    for (auto &MI : MBB) {


Account for block (and function) alignment?

Thank you very much for comment. I've implemented function size calculation like it is done in BranchRelaxation. Also I corrected insertIndirectBranch function.

arsenm · 2024-10-30T01:30:08Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

@@ -258,13 +258,32 @@ void XtensaFrameLowering::determineCalleeSaves(MachineFunction &MF,

 static unsigned estimateFunctionSizeInBytes(const MachineFunction &MF,
                                            const XtensaInstrInfo &TII) {
-  unsigned FnSize = 0;
+  const Align FunctiontAlignment = MF.getAlignment();


Typo Functiont?

arsenm · 2024-10-30T01:30:18Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+  const Align FunctiontAlignment = MF.getAlignment();
+  /// Offset - Distance from the beginning of the function to the end
+  /// of the basic block.
+  unsigned Offset = 0;


Probably should use int64_t

arsenm · 2024-10-30T01:31:32Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+      // The alignment of this MBB is larger than the function's alignment, so
+      // we can't tell whether or not it will insert nops. Assume that it will.
+      OffsetBB = alignTo(Offset, Alignment) + Alignment.value() -
+                 FunctiontAlignment.value();


Is this handled somewhere else already? Should this whole size estimate function be moved to a common helper function?

I moved this function to XtensaUtils.cpp.

I mean outside of the backend, a generic codegen utility.

Was this edge case already handled in other code? Is there a test for it?

I'm trying to find code which may be substituted by such function. Currently I found couple of places in backends:

llvm-project/llvm/lib/Target/LoongArch/LoongArchFrameLowering.cpp

Line 122 in ca1154d

static uint64_t estimateFunctionSizeInBytes(const LoongArchInstrInfo *TII,

and

llvm-project/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp

Line 1216 in ca1154d

static unsigned estimateFunctionSizeInBytes(const MachineFunction &MF,

This backends use more simple approach to caclulate function size.

And also there is a code in BranchRelaxation use similar approach to calculate block offsets, but it doesn't need function size:

llvm-project/llvm/lib/CodeGen/BranchRelaxation.cpp

Line 68 in a518ed2

unsigned postOffset(const MachineBasicBlock &MBB) const {

Should we find a code in other places, which can benefit by using such function before placing it in generic utils? Or maybe leave this function it in Xtensa backend?

This is a mess but I see RISCV is doing some target specific hacking in there I don't understand. The LoongArch one looks like a buggier version of the generic logic

I mean outside of the backend, a generic codegen utility.

Was this edge case already handled in other code? Is there a test for it?

I added new test for branch relaxation. May I ask your for advice about appropriate place for codegen utility? Should I place it in support or maybe add it to some class?

Probably directly in MachineFunction

arsenm · 2024-11-05T00:03:29Z

llvm/lib/Target/Xtensa/XtensaInstrInfo.cpp

+
+    int FrameIndex = XtensaFI->getBranchRelaxationScratchFrameIndex();
+    if (FrameIndex == -1)
+      report_fatal_error("underestimated function size");


Make message more specific

arsenm · 2024-11-05T00:03:48Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+      // The alignment of this MBB is larger than the function's alignment, so
+      // we can't tell whether or not it will insert nops. Assume that it will.
+      OffsetBB = alignTo(Offset, Alignment) + Alignment.value() -
+                 FunctiontAlignment.value();


Probably directly in MachineFunction

arsenm · 2024-11-05T21:16:39Z

llvm/lib/CodeGen/MachineFunction.cpp

+      // The alignment of this MBB is larger than the function's alignment, so
+      // we can't tell whether or not it will insert nops. Assume that it will.


I'm wondering if this should be a verifier error

You mean that this case shouldn't happen and verifier should throw an error? in such situation? Can we implement this in later commits (if we need this)?

Yes. I'm not sure it makes sense to have a block with less alignment than the function. you'd have to have some strange code realignment?

I haven't seen an example for Xtensa at this time with block alignment large then function. Actually I implemented alignment handling like it is done in Branch Relaxation pass in postOffset function. I thought it may be useful for general case.

llvm-project/llvm/lib/CodeGen/BranchRelaxation.cpp

Line 68 in a518ed2

unsigned postOffset(const MachineBasicBlock &MBB) const {

Implement insertIndirectBranch function and other functions needed by Branch Relaxation pass. Also implement estimateFunctionSizeInBytes function in MachineFunction class.

s-barannikov · 2024-11-08T11:30:55Z

llvm/lib/Target/Xtensa/XtensaInstrInfo.cpp

+  MRI.clearVirtRegs();
+}
+
+unsigned XtensaInstrInfo::insertConstBranchAtInst(


This method is unused and it compiles with a warning.

MaskRay · 2024-11-10T17:55:54Z

llvm/lib/Target/Xtensa/XtensaInstrInfo.cpp

+    MachineBasicBlock &MBB, MachineInstr *I, int64_t offset,
+    ArrayRef<MachineOperand> Cond, DebugLoc DL, int *BytesAdded) const {
+  // Shouldn't be a fall through.
+  assert(&MBB && "InsertBranch must not be told to insert a fallthrough");


This led to a Clang warning -Wundefined-bool-conversion. We ensure that the code base is warning free for recent Clang versions. Fixed at head.

Personally I use https://raw.githubusercontent.com/chromium/chromium/main/tools/clang/scripts/update.py

andreisfr added the backend:Xtensa label Oct 23, 2024

andreisfr requested review from arsenm and s-barannikov October 23, 2024 12:39

tom-borcin mentioned this pull request Oct 23, 2024

Tracking Issue for merging to upstream (LLVM-57) espressif/llvm-project#4

Open

andreisfr force-pushed the implement_branch_relaxation branch from 77d3f45 to bf10a83 Compare October 23, 2024 16:01

arsenm reviewed Oct 25, 2024

View reviewed changes

arsenm reviewed Oct 30, 2024

View reviewed changes

arsenm reviewed Nov 5, 2024

View reviewed changes

arsenm approved these changes Nov 5, 2024

View reviewed changes

arsenm approved these changes Nov 6, 2024

View reviewed changes

[Xtensa] Implement branch relaxation.

0460557

Implement insertIndirectBranch function and other functions needed by Branch Relaxation pass. Also implement estimateFunctionSizeInBytes function in MachineFunction class.

andreisfr force-pushed the implement_branch_relaxation branch from a444fa5 to 0460557 Compare November 7, 2024 08:19

andreisfr merged commit 3b1b127 into llvm:main Nov 7, 2024
8 checks passed

s-barannikov reviewed Nov 8, 2024

View reviewed changes

MaskRay added a commit that referenced this pull request Nov 10, 2024

[Xtensa] Fix Clang -Wundefined-bool-conversion after #113450

7111d03

MaskRay reviewed Nov 10, 2024

View reviewed changes

Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024

[Xtensa] Implement support for the BranchRelaxation. (llvm#113450)

29f51d6

Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024

[Xtensa] Fix Clang -Wundefined-bool-conversion after llvm#113450

fab9c00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Xtensa] Implement support for the BranchRelaxation. #113450

[Xtensa] Implement support for the BranchRelaxation. #113450

andreisfr commented Oct 23, 2024

github-actions bot commented Oct 23, 2024 •

edited

Loading

arsenm Oct 25, 2024

andreisfr Oct 29, 2024

arsenm Oct 30, 2024

andreisfr Oct 30, 2024

arsenm Oct 30, 2024

andreisfr Oct 30, 2024

arsenm Oct 30, 2024

andreisfr Oct 30, 2024

arsenm Oct 30, 2024

andreisfr Oct 30, 2024

arsenm Oct 30, 2024

andreisfr Nov 3, 2024

arsenm Nov 5, 2024

arsenm Nov 5, 2024

arsenm Nov 5, 2024

arsenm Nov 5, 2024

andreisfr Nov 6, 2024 •

edited

Loading

arsenm Nov 6, 2024

andreisfr Nov 6, 2024

s-barannikov Nov 8, 2024

MaskRay Nov 10, 2024 •

edited

Loading

		// The alignment of this MBB is larger than the function's alignment, so
		// we can't tell whether or not it will insert nops. Assume that it will.

[Xtensa] Implement support for the BranchRelaxation. #113450

[Xtensa] Implement support for the BranchRelaxation. #113450

Conversation

andreisfr commented Oct 23, 2024

github-actions bot commented Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreisfr Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaskRay Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2024 •

edited

Loading

andreisfr Nov 6, 2024 •

edited

Loading

MaskRay Nov 10, 2024 •

edited

Loading