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

[Xtensa] Lowering FRAMEADDR/RETURNADDR operations. #107363

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

andreisfr
Copy link
Contributor

No description provided.

@andreisfr andreisfr force-pushed the xtensa_lower_frameaddr_returnaddr branch from 35d700d to a210ae9 Compare September 5, 2024 08:07
@andreisfr
Copy link
Contributor Author

@arsenm , @s-barannikov , PTAL to this PR whenever you have the time.

MachineFunction &MF = DAG.getMachineFunction();
MachineFrameInfo &MFI = MF.getFrameInfo();
EVT VT = Op.getValueType();
unsigned RA = Xtensa::A0;
Copy link
Contributor

Choose a reason for hiding this comment

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

No reason to use this variable (but it should be Register/MCRegister)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comments. Fixed.

llvm/lib/Target/Xtensa/XtensaISelLowering.cpp Outdated Show resolved Hide resolved
// check the depth
// TODO: xtensa-gcc can handle this, by navigating through the stack, we
// should be able to do this too
assert((cast<ConstantSDNode>(Op.getOperand(0))->getZExtValue() == 0) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't assert on this, it's not invalid IR. Langref states it returns "zero if it cannot be identified"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I hope that I understand correctly your comment,

SDValue XtensaTargetLowering::LowerFRAMEADDR(SDValue Op,
SelectionDAG &DAG) const {
// check the depth
assert((cast<ConstantSDNode>(Op.getOperand(0))->getZExtValue() == 0) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

; CHECK-NEXT: ret
%1 = call ptr @llvm.returnaddress(i32 0)
ret ptr %1
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Test the non-0 cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new tests

; CHECK-LABEL: test_frameaddress_0:
; CHECK: or a2, a1, a1
; CHECK-NEXT: ret
%1 = call ptr @llvm.frameaddress(i32 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use named values in tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

@@ -0,0 +1,22 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
; RUN: llc -mtriple=xtensa -verify-machineinstrs < %s \
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 need -verify-machineinstrs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

EVT VT = Op.getValueType();
SDLoc DL(Op);

unsigned FrameRegister = Subtarget.getRegisterInfo()->getFrameRegister(MF);
Copy link
Contributor

Choose a reason for hiding this comment

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

Register

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

return SDValue();

MachineFunction &MF = DAG.getMachineFunction();
MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo();
MachineFrameInfo &MFI = MF.getFrameInfo();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// An index of one to the parent's return address, and so on.
// Depths > 0 not supported yet!
if (Op.getConstantOperandVal(0) > 0)
return SDValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

!= 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

// An index of zero corresponds to the current function's frame address.
// An index of one to the parent's frame address, and so on.
// Depths > 0 not supported yet!
if (Op.getConstantOperandVal(0) > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

!= 0

@andreisfr andreisfr merged commit 0ba8b24 into llvm:main Sep 13, 2024
8 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.

2 participants