[PATCH] [Xtensa] Implement FrameLowering methods and stack operation lowering. #92960

andreisfr · 2024-05-21T20:18:36Z

Implement emitPrologue/emitEpilogue methods, determine/spill/restore callee saved registers functionality with test. Also implement lowering of the DYNAMIC_STACKALLOC/STACKSAVE/STACKRESTORE stack operations with tests.

github-actions · 2024-05-21T20:21:16Z

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

… lowering. Implement emitPrologue/emitEpilogue methods, determine/spill/restore callee saved registers functionality with test. Also implement lowering of the DYNAMIC_STACKALLOC/STACKSAVE/STACKRESTORE stack operations with tests.

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

arsenm · 2024-05-22T10:40:06Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+    // Find the first instruction that restores a callee-saved register.
+    MachineBasicBlock::iterator I = MBBI;
+
+    for (unsigned i = 0; i < MFI.getCalleeSavedInfo().size(); ++i)


I don't see how incrementing the instruction iteration based on this makes sense

Thank you very much for review. I've made some code improvements (added more comments and loop is changed), I hope that I understand correctly your comment. Actually I used the same approach as in Mips architecture

llvm-project/llvm/lib/Target/Mips/MipsSEFrameLowering.cpp

Line 711 in 57c10fa

for (unsigned i = 0; i < MFI.getCalleeSavedInfo().size(); ++i)

, because it is suitable for Xtensa.

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

llvm/lib/Target/Xtensa/XtensaISelLowering.cpp

Remove redundant code from XtensaFrameLowering and XtensaISelLowering, add comments.

arsenm · 2024-05-28T09:34:35Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+
+  const std::vector<CalleeSavedInfo> &CSI = MFI.getCalleeSavedInfo();
+
+  if (CSI.size()) {


!CSI.empty()

arsenm · 2024-05-28T09:35:01Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+    // Find the instruction past the last instruction that saves a
+    // callee-saved register to the stack.
+    for (unsigned i = 0; i < CSI.size(); ++i)
+      ++MBBI;


This seems like a dubious way to guarantee this. Can you at least assert each instruction is a spill of the appropriate register?

I added check functions for store/restore instructions, PTAL.

arsenm · 2024-05-28T09:35:23Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+    // Find the first instruction at the end that restores a callee-saved
+    // register.
+    for (auto &Info : MFI.getCalleeSavedInfo())
+      --I;


Same as above, can this at least assert the instructions are the expected ones?

I added check functions

arsenm · 2024-05-28T09:37:58Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+  Align Alignment = TRI->getSpillAlign(RC);
+  int FI = MF.getFrameInfo().CreateStackObject(Size, Alignment, false);
+
+  RS->addScavengingFrameIndex(FI);


I'm not sure it's safe to do this in determineCalleeSaves. Can you create the emergency scavenging index in processFunctionBeforeFrameFinalized instead?

I moved frame index scavenging to processFunctionBeforeFrameFinalized

…emitEpilogue. Check whether store/restore instruciton for callee-saved registers are placed correctly in emitProlgue and emitEpilogue. Also move frame index scavenging from determineCalleeSaves function to processFunctionBeforeFrameFinalized function.

arsenm · 2024-05-29T10:42:28Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+static bool checkStoreInstruction(MachineBasicBlock::iterator I,
+                                  const std::vector<CalleeSavedInfo> &CSI) {


Name is too general. Also, this is way more code than I expected for this assert. Don't you expect these instructions to appear in order?

Can you use TII->isStoreToStackSlot and isCopyInstr to avoid special casing these specific opcodes?

Yes, I will implement then isStoreToStackSlot /isLoadFromStackSlot functions. Current PrologueEpilogueInsterter implementation places stores in direct order:
.. for (const CalleeSavedInfo &CS : CSI)..
and uses reverse order for placement loads at the end of basic block. So, we can assume that we always have such order and create redundant version which checks instructions. Is it acceptable?

If you're relying on seeing these exact instructions, it's OK to rely on the order. The point of the assert was to verify the instructions exactly match what you expected, which includes the order

I implemented isStoreToStackSlot /isLoadFromStackSlot functions and simplified callee-saved instructions checking.

arsenm · 2024-05-30T08:36:08Z

llvm/lib/Target/Xtensa/XtensaInstrInfo.cpp

+      return MI.getOperand(0).getReg();
+    }
+  }
+  return 0;


return Register()

arsenm · 2024-05-30T08:36:15Z

llvm/lib/Target/Xtensa/XtensaInstrInfo.cpp

+      return MI.getOperand(0).getReg();
+    }
+  }
+  return 0;


arsenm · 2024-05-30T14:01:36Z

llvm/lib/Target/Xtensa/XtensaFrameLowering.cpp

+#ifndef NDEBUG
+      // Checking that the instruction is exactly as expected
+      bool IsStoreInst = false;
+      if ((MBBI->getOpcode() == TargetOpcode::COPY) && Info.isSpilledToReg()) {


don't need these braces

I corrected "if-condition", is it what you mean? Also, I'm sorry, I changed loop structure a bit with callee-saved register checks to avoid compiler messages like "unused variable" and fix variable name typo. Could you PTAL? Thanks

MaskRay · 2024-06-04T23:49:22Z

I fixed two warnings in dd82fd4. You might want to build llvm-project with a not-too-ancient clang to ensure that there is no such warning for future patches:)

[PATCH] in the title is not needed. The majority of patches omit the trailing full stop in the title.

andreisfr · 2024-06-05T05:18:25Z

I fixed two warnings in dd82fd4. You might want to build llvm-project with a not-too-ancient clang to ensure that there is no such warning for future patches:)

[PATCH] in the title is not needed. The majority of patches omit the trailing full stop in the title.
Thank you very much, I'll take this into account.

sstefan1 requested review from s-barannikov, arsenm and MaskRay May 22, 2024 10:28

andreisfr force-pushed the xtensa_prologue_epilogue branch from 316fb85 to 78c9f32 Compare May 22, 2024 10:35

arsenm reviewed May 22, 2024

View reviewed changes

gerekon mentioned this pull request May 23, 2024

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

Open

[PATCH] [Xtensa] Minor fixes in FrameLowering and ISelLowering.

c84f7f1

Remove redundant code from XtensaFrameLowering and XtensaISelLowering, add comments.

arsenm reviewed May 28, 2024

View reviewed changes

arsenm reviewed May 29, 2024

View reviewed changes

[PATCH] [Xtensa] Simplify calle-saved register checking.

7df895e

arsenm reviewed May 30, 2024

View reviewed changes

arsenm approved these changes May 30, 2024

View reviewed changes

[PATCH] [Xtensa] Minor changes in callee-saved registers checks

039f075

arsenm approved these changes May 30, 2024

View reviewed changes

[PATCH] [Xtensa] Fix compile warnings anf variable name typo.

7a15a99

arsenm approved these changes May 30, 2024

View reviewed changes

sstefan1 merged commit 78f5d9c into llvm:main Jun 4, 2024
5 of 7 checks passed

MaskRay added a commit that referenced this pull request Jun 4, 2024

[Xtensa] Fix Clang warnings after #92960

dd82fd4

gerekon mentioned this pull request Dec 10, 2024

[Clang][Xtensa] Add Xtensa target. #118008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PATCH] [Xtensa] Implement FrameLowering methods and stack operation lowering. #92960

[PATCH] [Xtensa] Implement FrameLowering methods and stack operation lowering. #92960

andreisfr commented May 21, 2024

github-actions bot commented May 21, 2024 •

edited

Loading

arsenm May 22, 2024

andreisfr May 27, 2024

arsenm May 28, 2024

andreisfr May 28, 2024

arsenm May 28, 2024

andreisfr May 28, 2024

arsenm May 28, 2024

andreisfr May 28, 2024

arsenm May 28, 2024

andreisfr May 28, 2024

arsenm May 29, 2024

andreisfr May 29, 2024

arsenm May 29, 2024

andreisfr May 30, 2024

arsenm May 30, 2024

arsenm May 30, 2024

arsenm May 30, 2024

andreisfr May 30, 2024 •

edited

Loading

MaskRay commented Jun 4, 2024 •

edited

Loading

andreisfr commented Jun 5, 2024


		const std::vector<CalleeSavedInfo> &CSI = MFI.getCalleeSavedInfo();

		if (CSI.size()) {

		static bool checkStoreInstruction(MachineBasicBlock::iterator I,
		const std::vector<CalleeSavedInfo> &CSI) {

[PATCH] [Xtensa] Implement FrameLowering methods and stack operation lowering. #92960

[PATCH] [Xtensa] Implement FrameLowering methods and stack operation lowering. #92960

Conversation

andreisfr commented May 21, 2024

github-actions bot commented May 21, 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

Choose a reason for hiding this comment

andreisfr May 30, 2024 • edited Loading

Choose a reason for hiding this comment

MaskRay commented Jun 4, 2024 • edited Loading

andreisfr commented Jun 5, 2024

github-actions bot commented May 21, 2024 •

edited

Loading

andreisfr May 30, 2024 •

edited

Loading

MaskRay commented Jun 4, 2024 •

edited

Loading