Page MenuHomePhabricator

AMDGPU: RETURNADDR lowering
ClosedPublic

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

Details

Diff Detail

Repository
rL LLVM

Event Timeline

aakanksha555 created this revision.Mar 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
1 ↗(On Diff #191776)

Should use HSA triple

9 ↗(On Diff #191776)

Use named values

12 ↗(On Diff #191776)

Needs test with non-0 frame

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
7 ↗(On Diff #191776)

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

aakanksha555 marked 4 inline comments as done.
arsenm added inline comments.May 8 2019, 5:36 AM
lib/Target/AMDGPU/SIISelLowering.cpp
4156 ↗(On Diff #198542)

Formatting

4159–4161 ↗(On Diff #198542)

This can just give 0 instead of erring

4168 ↗(On Diff #198542)

This still won't handle kernels correctly

test/CodeGen/AMDGPU/returnaddress.ll
19 ↗(On Diff #198542)

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 marked 5 inline comments as done.

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

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

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?

Minor changes in one of the tests.

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
47–49 ↗(On Diff #199841)

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

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

arsenm added inline comments.May 24 2019, 1:54 PM
returnadr.diff
2 ↗(On Diff #201329)

You somehow added the diff as a file

105 ↗(On Diff #201329)

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 ↗(On Diff #201329)

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

Fix in one of the tests.

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

LGTM

test/CodeGen/AMDGPU/returnaddress.ll
60–62 ↗(On Diff #201335)

This can just be one block

This revision is now accepted and ready to land.May 24 2019, 2:18 PM
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