Page MenuHomePhabricator

AMDGPU: RETURNADDR lowering
ClosedPublic

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

Details

Diff Detail

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

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

Unnecessary comment

2807–2810

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

2815

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

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 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
19

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

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
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

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
43–44

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
61–63

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