This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU : Add Clang builtin intrinsics for compare with the full wavefront result
ClosedPublic

Authored by wdng on Jul 28 2016, 1:46 PM.

Diff Detail

Event Timeline

wdng updated this revision to Diff 65984.Jul 28 2016, 1:46 PM
wdng retitled this revision from to AMDGPU : Add Clang builtin intrinsics for compare with the full wavefront result.
wdng updated this object.
wdng added reviewers: arsenm, tstellarAMD.
arsenm edited edge metadata.Jul 28 2016, 1:50 PM

Tests should be added for non-constant compare type operands being rejected in test/CodeGenOpenCL/builtins-amdgcn-error.cl

include/clang/Basic/BuiltinsAMDGPU.def
73–74

There should be 4 intrinsics defined for icmp. We need uicmp, uicmpl, sicmp, sicmpl. I'm not sure what W is.

74

The bultin suffix for long should be l. The other builtins also use L instead of W for int64

75–76

All of these intrinsics should have the final parameter marked with I to require it be an immediate

test/CodeGenOpenCL/builtins-amdgcn.cl
207

Non-constant 3rd parameter is invalid

wdng added inline comments.Jul 28 2016, 1:56 PM
include/clang/Basic/BuiltinsAMDGPU.def
73–74

According to the definition on https://code.woboq.org/llvm/clang/include/clang/Basic/Builtins.def.html W is for int64_t

arsenm added inline comments.Jul 28 2016, 1:58 PM
include/clang/Basic/BuiltinsAMDGPU.def
73–74

L is for long which is also 64-bit, we don't need to use W

test/CodeGenOpenCL/builtins-amdgcn.cl
7

This should be removed

wdng marked 4 inline comments as done.Jul 28 2016, 2:11 PM
wdng added inline comments.
test/CodeGenOpenCL/builtins-amdgcn.cl
207

should the third constant be "const int c" ?

arsenm added inline comments.Jul 28 2016, 2:13 PM
test/CodeGenOpenCL/builtins-amdgcn.cl
207

Yes

207

Maybe unsigned

wdng updated this revision to Diff 66032.Jul 28 2016, 3:07 PM
wdng edited edge metadata.
wdng marked 3 inline comments as done.
  1. Modified intrinsic parameters' data type
  2. Modified the LIT test functions' last parameter to const uint.
wdng updated this revision to Diff 66044.Jul 28 2016, 3:39 PM

Fixed ref assembly errors.

arsenm added inline comments.Jul 28 2016, 3:53 PM
include/clang/Basic/BuiltinsAMDGPU.def
73–78

The return types look wrong. They should all be ulong, not signed

test/CodeGenOpenCL/builtins-amdgcn.cl
210

The tests seem to be in a random order. Can you sort them so that the same operation on the same type is the next one? This is missing uicmp_i32

212

The output type should be ulong

214

This isn't a valid icmp predicate type. Ideally there would be an error for this but that can be another patch

228

This test is using an unsigned predicate value for the signed version

wdng updated this revision to Diff 66134.Jul 29 2016, 9:27 AM
wdng marked 3 inline comments as done.
  1. Change return data type to unsigned long.
  2. Fix incorrect use of cond code.
arsenm added inline comments.Jul 29 2016, 10:08 AM
include/clang/Basic/BuiltinsAMDGPU.def
75–76

This looks inconsistent. You don't need the 'S'es

wdng updated this revision to Diff 66143.Jul 29 2016, 10:26 AM

Removed 'S'es.

Still missing test that non-constants for 3rd operand are rejected

test/CodeGenOpenCL/builtins-amdgcn.cl
233

Unused c parameter

wdng marked an inline comment as done.Aug 1 2016, 9:08 AM

Still missing test that non-constants for 3rd operand are rejected

If I write a test with the third parameter as an non-constant, it fails at the compilation phase:

llvm/tools/clang/test/CodeGenOpenCL/builtins-amdgcn.cl:249:10: error: argument to '__builtin_amdgcn_fcmp' must be a constant integer

*out = __builtin_amdgcn_fcmp(a, b, c);
       ^                           ~

1 error generated.
FileCheck error: '-' is empty.

So how to write a FileCheck to detect this? Thanks!

arsenm added a comment.Aug 1 2016, 8:23 PM
In D22934#502183, @wdng wrote:

Still missing test that non-constants for 3rd operand are rejected

If I write a test with the third parameter as an non-constant, it fails at the compilation phase:

llvm/tools/clang/test/CodeGenOpenCL/builtins-amdgcn.cl:249:10: error: argument to '__builtin_amdgcn_fcmp' must be a constant integer

*out = __builtin_amdgcn_fcmp(a, b, c);
       ^                           ~

1 error generated.
FileCheck error: '-' is empty.

So how to write a FileCheck to detect this? Thanks!

Yes, that's the point. The tests in builtins-amdgcn-error.cl do this, you should add these tests there

wdng updated this revision to Diff 66484.Aug 2 2016, 8:55 AM

A failure test is added.

arsenm added inline comments.Aug 2 2016, 8:10 PM
test/CodeGenOpenCL/builtins-amdgcn-error.cl
20–23 ↗(On Diff #66484)

All of the new builtins should be tested here

wdng updated this revision to Diff 66703.EditedAug 3 2016, 1:33 PM
wdng marked an inline comment as done.

Add all of the new builtins fail tests.

arsenm accepted this revision.Aug 4 2016, 1:59 PM
arsenm edited edge metadata.

LGTM with the last test cleanup

test/CodeGenOpenCL/builtins-amdgcn-error.cl
21 ↗(On Diff #66703)

It doesn't really matter, but the parameter types here should be int.

31 ↗(On Diff #66703)

Same, should be long

This revision is now accepted and ready to land.Aug 4 2016, 1:59 PM
This revision was automatically updated to reflect the committed changes.
jdoerfert added a subscriber: jdoerfert.EditedSep 17 2022, 4:42 PM

Wrong review/tab. Apologies.

Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2022, 4:42 PM