This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Allow flexible register names in inline asm constraints
ClosedPublic

Authored by yaxunl on Sep 7 2017, 6:58 AM.

Details

Summary

Currently AMDGPU inline asm only allow "v" and "s" as register names in constraints.

This patch allows the following register names in constraints: (n, m is unsigned integer, n < m)

v

s

{vn} or {v[n]}

{sn} or {s[n]}

{S} , where S is a special register name

{v[n:m]}

{s[n:m]}

Diff Detail

Event Timeline

yaxunl created this revision.Sep 7 2017, 6:58 AM
arsenm added inline comments.Sep 7 2017, 10:04 AM
test/Sema/inline-asm-validate-amdgpu.cl
26

I don't understand. The backend parsed syntax is v[3:4]. Why should v3:4 be valid? Also in this example you are using a 64-bit input to a 32-bit operand

arsenm added inline comments.Sep 7 2017, 10:09 AM
test/Sema/inline-asm-validate-amdgpu.cl
26

i.e. this won't parse in the backend and is invalid. The brackets are required

yaxunl added inline comments.Sep 7 2017, 11:11 AM
test/Sema/inline-asm-validate-amdgpu.cl
26

Will fix the syntax about [].

The test did not consider validity of the inline assembly for the backend since FE only check format. I agree it is better to make it valid for backend and will try to fix that.

yaxunl updated this revision to Diff 114223.Sep 7 2017, 11:42 AM
yaxunl marked 3 inline comments as done.
yaxunl edited the summary of this revision. (Show Details)

Revised by Matt's comments.

b-sumner edited edge metadata.Sep 7 2017, 12:18 PM

The assembler accepts v[N] in addition to vN. I'm not sure if that is needed here.

The assembler accepts v[N] in addition to vN. I'm not sure if that is needed here.

Then we'd better also allow that in constraints to avoid confusion of users.

yaxunl updated this revision to Diff 114367.Sep 8 2017, 8:12 AM
yaxunl edited the summary of this revision. (Show Details)

Allow {v[n]} and {s[n]}. Add more tests.

arsenm added inline comments.Sep 15 2017, 10:40 AM
lib/Basic/Targets/AMDGPU.h
69

Typo wheere

91

I'm not sure I understand these data() - 1s.

test/Sema/inline-asm-validate-amdgpu.cl
55

While you're here can we add some tests for the immediate constraints? There was a bug report recently when using s_trap with the i constraints for the constant operand.

yaxunl marked 2 inline comments as done.Sep 15 2017, 12:09 PM
yaxunl added inline comments.
lib/Basic/Targets/AMDGPU.h
91

The caller of this function expects Name is on the last parsed char and will increase it before check the next char.

test/Sema/inline-asm-validate-amdgpu.cl
55

what's the syntax of the immediate constraints? And some examples? Thanks.

yaxunl updated this revision to Diff 116383.Sep 22 2017, 12:04 PM
yaxunl marked 4 inline comments as done.
yaxunl edited the summary of this revision. (Show Details)

Fix typo.

yaxunl marked an inline comment as done.Sep 22 2017, 12:06 PM
yaxunl added inline comments.
test/Sema/inline-asm-validate-amdgpu.cl
55

Ping. Can we leave this for another patch? Since there are people waiting for this feature.

yaxunl marked an inline comment as done.

Ping.

Brian, Stas,

Can you review this since Matt is on vacation? Thanks.

b-sumner accepted this revision.Sep 28 2017, 11:04 AM

LGTM. I think we can leave immediates to another patch.

This revision is now accepted and ready to land.Sep 28 2017, 11:04 AM
This revision was automatically updated to reflect the committed changes.