Page MenuHomePhabricator

AMDGPU: Make hidden argument metadata consistent with amdgpu-implicitarg-num-bytes attribute

Authored by kzhuravl on Jul 9 2018, 12:09 PM.

Diff Detail

Event Timeline

kzhuravl created this revision.Jul 9 2018, 12:09 PM
yaxunl added inline comments.Jul 9 2018, 12:27 PM

Do we need to check this?

What if a non-OpenCL program wants implicit kernel argument?

Can we remove the dependence on opencl.ocl.version here?

arsenm added inline comments.Jul 9 2018, 12:35 PM

I thought the whole point was to remove this check

arsenm added inline comments.Jul 9 2018, 12:39 PM

Missing space

kzhuravl updated this revision to Diff 154682.Jul 9 2018, 1:05 PM

Drop checks for OpenCL. I was not aware that it was decided to possibly allow this for non-OpenCL languages.

kzhuravl updated this revision to Diff 154683.Jul 9 2018, 1:06 PM
kzhuravl marked 3 inline comments as done.

Add missing space.

yaxunl accepted this revision.Jul 9 2018, 1:34 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Jul 9 2018, 1:34 PM
t-tye added inline comments.Jul 9 2018, 6:41 PM

Just checking if this is correct for Mesa too?

arsenm added inline comments.Jul 10 2018, 1:21 AM

Clover doesn't use this attribute. I'm not sure what the current state is, but it follow some frankenstein ABI between HSA and what it was doing before. Since it isn't really maintained I don't think we need to worry too much about it

This revision was automatically updated to reflect the committed changes.
t-tye added inline comments.Jul 10 2018, 9:31 AM

I thought Mesa was using this attribute and setting it to 16. But @kzhuravl points out that this code is only called for the AMDHSA os and so it is not an issue.