This is an archive of the discontinued LLVM Phabricator instance.

Differential D59666

AMDGPU: RETURNADDR lowering
ClosedPublic

Authored by aakanksha555 on Mar 21 2019, 2:04 PM.

Details

Reviewers
arsenm
b-sumner
Commits
rGd5443f8c21b1: AMDGPU: Return address lowering
rL362001: AMDGPU: Return address lowering
Summary

Patch to do return address lowering.

Diff Detail

Event Timeline

aakanksha555 created this revision.Mar 21 2019, 2:04 PM
Herald added a subscriber: jdoerfert. · View Herald TranscriptMar 21 2019, 2:04 PM
aakanksha555 retitled this revision from HIP compiler option -finstrument-functions flag to AMDGPU: HIP compiler option -finstrument-functions flag.Mar 21 2019, 2:10 PM
arsenm added inline comments.Mar 21 2019, 2:11 PM
lib/Target/AMDGPU/AMDGPUISelLowering.cpp
2793–2795 ↗(On Diff #191776)

verifyReturnAddressArgumentIsConstant should probably be removed in a separate patch. The verifier now rejects the IR, and the DAG should never be allowed to produce an invalid one

2799–2801 ↗(On Diff #191776)

Unnecessary comment

2807–2810 ↗(On Diff #191776)

This should not be an assert. According to the spec, this could just return 0 for another frame id

2815 ↗(On Diff #191776)

Should use TRI::getReturnAddressReg rather than hardcoding it in another place. You can just hardcode the register class though

test/CodeGen/AMDGPU/returnaddress.ll
2

Should use HSA triple

10

Use named values

13

Needs test with non-0 frame

arsenm added a comment.Mar 21 2019, 2:11 PM

Commit message should also say it's lowering returnaddress, not mention instrument-functions

arsenm added inline comments.Mar 21 2019, 8:24 PM
test/CodeGen/AMDGPU/returnaddress.ll
8

Needs a test with amdgpu_kernel and shaders. As is this will fail as they don’t have return addresses

aakanksha555 updated this revision to Diff 198542.May 7 2019, 3:11 PM
aakanksha555 marked 4 inline comments as done.
arsenm added inline comments.May 8 2019, 5:36 AM
lib/Target/AMDGPU/SIISelLowering.cpp
4156

Formatting

4159–4161

This can just give 0 instead of erring

4168

This still won't handle kernels correctly

test/CodeGen/AMDGPU/returnaddress.ll
20

Needs tests with a kernel and shader, since this should crash as is since there is no return address

arsenm added inline comments.May 8 2019, 5:37 AM
lib/Target/AMDGPU/AMDGPUISelLowering.cpp
29 ↗(On Diff #198542)

Not sure why you need a new include here

aakanksha555 updated this revision to Diff 199493.May 14 2019, 12:42 PM
aakanksha555 marked 5 inline comments as done.

Return 0 for kernels and non-zero arguments to llvm.returnaddress.

arsenm added a comment.May 14 2019, 1:11 PM

Mostly LGTM, but 2 more test cases might be useful; one with no ret, and one with the use outside of the entry block

aakanksha555 updated this revision to Diff 199514.May 14 2019, 2:31 PM

Added a test outside entry block; doesn't make any difference in the output.
By a test with no ret, do you mean something that returns void?

aakanksha555 updated this revision to Diff 199841.May 16 2019, 8:56 AM

Minor changes in one of the tests.

arsenm added a comment.May 16 2019, 10:14 AM
In D59666#1502063, @aakanksha555 wrote:

Added a test outside entry block; doesn't make any difference in the output.
By a test with no ret, do you mean something that returns void?

No, I mean ends in unreachable

test/CodeGen/AMDGPU/returnaddress.ll
48–50

This is still in the entry block

aakanksha555 retitled this revision from AMDGPU: HIP compiler option -finstrument-functions flag to AMDGPU: RETURNADDR lowering.May 24 2019, 8:17 AM
aakanksha555 updated this revision to Diff 201329.May 24 2019, 1:52 PM

Fixed two tests; one which does not return and another with use outside of the entry block.

Herald added a subscriber: jfb. · View Herald TranscriptMay 24 2019, 1:52 PM
arsenm added inline comments.May 24 2019, 1:54 PM
returnadr.diff
2

You somehow added the diff as a file

105

The call is still in the entry block. You should also make it a conditional branch to make sure nothing folds it

test/CodeGen/AMDGPU/returnaddress.ll
42–43

The call is still in the entry block. You should also make it a conditional branch to make sure nothing folds it

aakanksha555 updated this revision to Diff 201335.May 24 2019, 2:14 PM

Fix in one of the tests.

arsenm accepted this revision.May 24 2019, 2:18 PM

LGTM

test/CodeGen/AMDGPU/returnaddress.ll
61–63

This can just be one block

This revision is now accepted and ready to land.May 24 2019, 2:18 PM
Closed by commit rL362001: AMDGPU: Return address lowering (authored by aakanksha555). · Explain WhyMay 29 2019, 11:19 AM
This revision was automatically updated to reflect the committed changes.
aakanksha555 marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 11:19 AM

Revision Contents

PathSize
lib/
Target/
AMDGPU/
2 lines
26 lines
127 lines
test/
CodeGen/
AMDGPU/
64 lines

Diff 201329

lib/Target/AMDGPU/SIISelLowering.h

Context not available.
SDValue LowerTrig(SDValue Op, SelectionDAG &DAG) const; SDValue LowerTrig(SDValue Op, SelectionDAG &DAG) const;
SDValue LowerATOMIC_CMP_SWAP(SDValue Op, SelectionDAG &DAG) const; SDValue LowerATOMIC_CMP_SWAP(SDValue Op, SelectionDAG &DAG) const;
SDValue LowerBRCOND(SDValue Op, SelectionDAG &DAG) const; SDValue LowerBRCOND(SDValue Op, SelectionDAG &DAG) const;
SDValue LowerRETURNADDR(SDValue Op, SelectionDAG &DAG) const;
SDValue adjustLoadValueType(unsigned Opcode, MemSDNode *M, SDValue adjustLoadValueType(unsigned Opcode, MemSDNode *M,
SelectionDAG &DAG, ArrayRef<SDValue> Ops, SelectionDAG &DAG, ArrayRef<SDValue> Ops,
bool IsIntrinsic = false) const; bool IsIntrinsic = false) const;
Context not available.

lib/Target/AMDGPU/SIISelLowering.cpp

Context not available.
switch (Op.getOpcode()) { switch (Op.getOpcode()) {
default: return AMDGPUTargetLowering::LowerOperation(Op, DAG); default: return AMDGPUTargetLowering::LowerOperation(Op, DAG);
case ISD::BRCOND: return LowerBRCOND(Op, DAG); case ISD::BRCOND: return LowerBRCOND(Op, DAG);
case ISD::RETURNADDR: return LowerRETURNADDR(Op, DAG);
case ISD::LOAD: { case ISD::LOAD: {
SDValue Result = LowerLOAD(Op, DAG); SDValue Result = LowerLOAD(Op, DAG);
assert((!Result.getNode() || assert((!Result.getNode() ||
Context not available.
return Chain; return Chain;
} }
arsenmUnsubmitted
Done
ReplyInline Actions

Formatting

arsenm: Formatting
SDValue SITargetLowering::LowerRETURNADDR(SDValue Op,
SelectionDAG &DAG) const {
MVT VT = Op.getSimpleValueType();
SDLoc DL(Op);
// Checking the depth
arsenmUnsubmitted
Done
ReplyInline Actions

This can just give 0 instead of erring

arsenm: This can just give 0 instead of erring
if (cast<ConstantSDNode>(Op.getOperand(0))->getZExtValue() != 0)
return DAG.getConstant(0, DL, VT);
MachineFunction &MF = DAG.getMachineFunction();
const SIMachineFunctionInfo *Info = MF.getInfo<SIMachineFunctionInfo>();
// Check for kernel and shader functions
if (Info->isEntryFunction())
arsenmUnsubmitted
Done
ReplyInline Actions

This still won't handle kernels correctly

arsenm: This still won't handle kernels correctly
return DAG.getConstant(0, DL, VT);
MachineFrameInfo &MFI = MF.getFrameInfo();
// There is a call to @llvm.returnaddress in this function
MFI.setReturnAddressIsTaken(true);
const SIRegisterInfo *TRI = getSubtarget()->getRegisterInfo();
// Get the return address reg and mark it as an implicit live-in
unsigned Reg = MF.addLiveIn(TRI->getReturnAddressReg(MF), getRegClassFor(VT));
return DAG.getCopyFromReg(DAG.getEntryNode(), DL, Reg, VT);
}
SDValue SITargetLowering::getFPExtOrFPTrunc(SelectionDAG &DAG, SDValue SITargetLowering::getFPExtOrFPTrunc(SelectionDAG &DAG,
SDValue Op, SDValue Op,
const SDLoc &DL, const SDLoc &DL,
Context not available.

returnadr.diff

  • This file was added.
diff --git a/lib/Target/AMDGPU/SIISelLowering.cpp b/lib/Target/AMDGPU/SIISelLowering.cpp
index c2cda5ef4d7..a59b54902b2 100644
arsenmUnsubmitted
Not Done
ReplyInline Actions

You somehow added the diff as a file

arsenm: You somehow added the diff as a file
--- a/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -3695,6 +3695,7 @@ SDValue SITargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
switch (Op.getOpcode()) {
default: return AMDGPUTargetLowering::LowerOperation(Op, DAG);
case ISD::BRCOND: return LowerBRCOND(Op, DAG);
+ case ISD::RETURNADDR: return LowerRETURNADDR(Op, DAG);
case ISD::LOAD: {
SDValue Result = LowerLOAD(Op, DAG);
assert((!Result.getNode() ||
@@ -4153,6 +4154,31 @@ SDValue SITargetLowering::LowerBRCOND(SDValue BRCOND,
return Chain;
}
+SDValue SITargetLowering::LowerRETURNADDR(SDValue Op,
+ SelectionDAG &DAG) const {
+ MVT VT = Op.getSimpleValueType();
+ SDLoc DL(Op);
+ // Checking the depth
+ if (cast<ConstantSDNode>(Op.getOperand(0))->getZExtValue() != 0)
+ return DAG.getConstant(0, DL, VT);
+
+ MachineFunction &MF = DAG.getMachineFunction();
+ const SIMachineFunctionInfo *Info = MF.getInfo<SIMachineFunctionInfo>();
+ // Check for kernel and shader functions
+ if (Info->isEntryFunction())
+ return DAG.getConstant(0, DL, VT);
+
+ MachineFrameInfo &MFI = MF.getFrameInfo();
+ // There is a call to @llvm.returnaddress in this function
+ MFI.setReturnAddressIsTaken(true);
+
+ const SIRegisterInfo *TRI = getSubtarget()->getRegisterInfo();
+ // Get the return address reg and mark it as an implicit live-in
+ unsigned Reg = MF.addLiveIn(TRI->getReturnAddressReg(MF), getRegClassFor(VT));
+
+ return DAG.getCopyFromReg(DAG.getEntryNode(), DL, Reg, VT);
+}
+
SDValue SITargetLowering::getFPExtOrFPTrunc(SelectionDAG &DAG,
SDValue Op,
const SDLoc &DL,
diff --git a/lib/Target/AMDGPU/SIISelLowering.h b/lib/Target/AMDGPU/SIISelLowering.h
index 60a474f51e5..227a409ebe2 100644
--- a/lib/Target/AMDGPU/SIISelLowering.h
+++ b/lib/Target/AMDGPU/SIISelLowering.h
@@ -89,7 +89,7 @@ private:
SDValue LowerTrig(SDValue Op, SelectionDAG &DAG) const;
SDValue LowerATOMIC_CMP_SWAP(SDValue Op, SelectionDAG &DAG) const;
SDValue LowerBRCOND(SDValue Op, SelectionDAG &DAG) const;
-
+ SDValue LowerRETURNADDR(SDValue Op, SelectionDAG &DAG) const;
SDValue adjustLoadValueType(unsigned Opcode, MemSDNode *M,
SelectionDAG &DAG, ArrayRef<SDValue> Ops,
bool IsIntrinsic = false) const;
diff --git a/test/CodeGen/AMDGPU/returnaddress.ll b/test/CodeGen/AMDGPU/returnaddress.ll
new file mode 100644
index 00000000000..9795d96dd07
--- /dev/null
+++ b/test/CodeGen/AMDGPU/returnaddress.ll
@@ -0,0 +1,64 @@
+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck --check-prefix=GCN %s
+
+; Test with zero frame
+; GCN-LABEL: {{^}}func1
+; GCN: v_mov_b32_e32 v0, s30
+; GCN: v_mov_b32_e32 v1, s31
+; GCN: s_setpc_b64 s[30:31]
+define i8* @func1() nounwind {
+entry:
+ %0 = tail call i8* @llvm.returnaddress(i32 0)
+ ret i8* %0
+}
+
+; Test with non-zero frame
+; GCN-LABEL: {{^}}func2
+; GCN: v_mov_b32_e32 v0, 0
+; GCN: v_mov_b32_e32 v1, 0
+; GCN: s_setpc_b64 s[30:31]
+define i8* @func2() nounwind {
+entry:
+ %0 = tail call i8* @llvm.returnaddress(i32 1)
+ ret i8* %0
+}
+
+; Test with amdgpu_kernel
+; GCN-LABEL: {{^}}func3
+; GCN: v_mov_b32_e32 v0, 0
+; GCN: v_mov_b32_e32 v1, v0
+define amdgpu_kernel void @func3(i8** %out) nounwind {
+entry:
+ %tmp = tail call i8* @llvm.returnaddress(i32 0)
+ store i8* %tmp, i8** %out, align 4
+ ret void
+}
+
+; Test without an entry-block
+; GCN-LABEL: {{^}}func4
+; GCN: v_mov_b32_e32 v0, 0
+; GCN: v_mov_b32_e32 v1, v0
+define amdgpu_kernel void @func4(i8** %out) nounwind {
+entry:
+ %tmp = tail call i8* @llvm.returnaddress(i32 1)
arsenmUnsubmitted
Not Done
ReplyInline Actions

The call is still in the entry block. You should also make it a conditional branch to make sure nothing folds it

arsenm: The call is still in the entry block. You should also make it a conditional branch to make sure…
+ br label %exit
+
+exit:
+ store i8* %tmp, i8** %out, align 4
+ ret void
+}
+
+; Test ending in unreachable
+; GCN-LABEL: {{^}}func5
+; GCN: v_mov_b32_e32 v0, 0
+define void @func5() nounwind {
+entry:
+ %tmp = tail call i8* @llvm.returnaddress(i32 2)
+ br label %unreachable
+
+unreachable:
+ store volatile i32 0, i32 addrspace(3)* undef, align 4
+ unreachable
+}
+
+
+declare i8* @llvm.returnaddress(i32) nounwind readnone

test/CodeGen/AMDGPU/returnaddress.ll

  • This file was added.
; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck --check-prefix=GCN %s
arsenmUnsubmitted
Not Done
ReplyInline Actions

Should use HSA triple

arsenm: Should use HSA triple
; Test with zero frame
; GCN-LABEL: {{^}}func1
; GCN: v_mov_b32_e32 v0, s30
; GCN: v_mov_b32_e32 v1, s31
; GCN: s_setpc_b64 s[30:31]
define i8* @func1() nounwind {
arsenmUnsubmitted
Not Done
ReplyInline Actions

Needs a test with amdgpu_kernel and shaders. As is this will fail as they don’t have return addresses

arsenm: Needs a test with amdgpu_kernel and shaders. As is this will fail as they don’t have return…
entry:
%0 = tail call i8* @llvm.returnaddress(i32 0)
arsenmUnsubmitted
Not Done
ReplyInline Actions

Use named values

arsenm: Use named values
ret i8* %0
}
arsenmUnsubmitted
Not Done
ReplyInline Actions

Needs test with non-0 frame

arsenm: Needs test with non-0 frame
; Test with non-zero frame
; GCN-LABEL: {{^}}func2
; GCN: v_mov_b32_e32 v0, 0
; GCN: v_mov_b32_e32 v1, 0
; GCN: s_setpc_b64 s[30:31]
define i8* @func2() nounwind {
entry:
arsenmUnsubmitted
Done
ReplyInline Actions

Needs tests with a kernel and shader, since this should crash as is since there is no return address

arsenm: Needs tests with a kernel and shader, since this should crash as is since there is no return…
%0 = tail call i8* @llvm.returnaddress(i32 1)
ret i8* %0
}
; Test with amdgpu_kernel
; GCN-LABEL: {{^}}func3
; GCN: v_mov_b32_e32 v0, 0
; GCN: v_mov_b32_e32 v1, v0
define amdgpu_kernel void @func3(i8** %out) nounwind {
entry:
%tmp = tail call i8* @llvm.returnaddress(i32 0)
store i8* %tmp, i8** %out, align 4
ret void
}
; Test with use outside the entry-block
; GCN-LABEL: {{^}}func4
; GCN: v_mov_b32_e32 v0, 0
; GCN: v_mov_b32_e32 v1, v0
define amdgpu_kernel void @func4(i8** %out) nounwind {
entry:
%tmp = tail call i8* @llvm.returnaddress(i32 1)
br label %exit
arsenmUnsubmitted
Not Done
ReplyInline Actions

The call is still in the entry block. You should also make it a conditional branch to make sure nothing folds it

arsenm: The call is still in the entry block. You should also make it a conditional branch to make sure…
exit:
store i8* %tmp, i8** %out, align 4
ret void
}
; Test ending in unreachable
arsenmUnsubmitted
Not Done
ReplyInline Actions

This is still in the entry block

arsenm: This is still in the entry block
; GCN-LABEL: {{^}}func5
; GCN: v_mov_b32_e32 v0, 0
define void @func5() nounwind {
entry:
%tmp = tail call i8* @llvm.returnaddress(i32 2)
br label %unreachable
unreachable:
store volatile i32 0, i32 addrspace(3)* undef, align 4
unreachable
}
arsenmUnsubmitted
Done
ReplyInline Actions

This can just be one block

arsenm: This can just be one block
declare i8* @llvm.returnaddress(i32) nounwind readnone