Page MenuHomePhabricator

Add builtin_elementwise_sin and builtin_elementwise_cos
ClosedPublic

Authored by bob80905 on Sep 30 2022, 10:15 PM.

Details

Summary

Add codegen for llvm cos and sin elementwise builtins
The sin and cos elementwise builtins are necessary for HLSL codegen.
Tests were added to make sure that the expected errors are encountered when these functions are given inputs of incompatible types.
The new builtins are restricted to floating point types only.

Diff Detail

Event Timeline

bob80905 created this revision.Sep 30 2022, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 10:15 PM
bob80905 requested review of this revision.Sep 30 2022, 10:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 10:15 PM
bob80905 updated this revision to Diff 464471.Sep 30 2022, 10:18 PM

rever format commit

bob80905 updated this revision to Diff 464472.Sep 30 2022, 10:21 PM
  • make format consistent, update test copy mistake
bob80905 updated this revision to Diff 464730.Oct 3 2022, 10:25 AM

run git clang-format on changes rather than whole document

fhahn added a comment.Oct 3 2022, 10:28 AM

Could you update the title to make it clear this adds new Clang builtins, not intrinsics (they are lowered to LLVM intrinsics). The behavior of the new builtins should be specified in LanguageExtensions.rst (https://clang.llvm.org/docs/LanguageExtensions.html#vector-builtins)

bob80905 retitled this revision from Add sin and cos llvm intrinsics to Add sin and cos llvm builtins.Oct 3 2022, 10:30 AM
bob80905 edited the summary of this revision. (Show Details)

Please clarify the commit message (review description) to better explain what it is you're doing here, the purpose/etc as well as the documentation requested by @fhahn

This also needs release notes.

clang/test/SemaCXX/builtins-elementwise-math.cpp
76

Nit: please make sure this newline is present.

dexonsmith resigned from this revision.Oct 3 2022, 10:50 AM
bob80905 updated this revision to Diff 464739.Oct 3 2022, 10:51 AM
  • add docs, add new line
bob80905 edited the summary of this revision. (Show Details)Oct 3 2022, 10:54 AM
bob80905 updated this revision to Diff 464744.Oct 3 2022, 11:01 AM
bob80905 edited the summary of this revision. (Show Details)
  • add release note

Does these support scalable vector types from ARM SVE or RISC-V? I can't remember what the rest of the __builtin_elementwise do. I ask because the backends will probably crash for them.

The titled of this patch should be something like "Add builtin_elementwise_sin and builtin_elementwise_cos".

Can you explain why this uses a new builtin name instead of overloading the existing builtins to work on vectors? I can imagine reasons why, but I think it needs to be explained. I can't imagine cos and sin not being element-wise operations on a vector.

The titled of this patch should be something like "Add builtin_elementwise_sin and builtin_elementwise_cos".

Can you explain why this uses a new builtin name instead of overloading the existing builtins to work on vectors? I can imagine reasons why, but I think it needs to be explained. I can't imagine cos and sin not being element-wise operations on a vector.

It just adds additional builtins following the other vector-wise builtins.
As mentioned in https://lists.llvm.org/pipermail/cfe-dev/2021-September/068999.html, it is much easier to just use one builtin for all overloads instead of using different builtins for different overloads.

bob80905 retitled this revision from Add sin and cos llvm builtins to Add builtin_elementwise_sin and builtin_elementwise_cos.Oct 3 2022, 4:42 PM

Ah, I'd forgotten we had a bunch of elementwise builtins already. I wouldn't be surprised if I asked for that in that patch, actually, especially because the existing ones include some functions like max and min where you could easily imagine them being cross-lane. Nevermind.

Does these support scalable vector types from ARM SVE or RISC-V? I can't remember what the rest of the __builtin_elementwise do. I ask because the backends will probably crash for them.

checkMathBuiltinElementType should stop scalable vector types in Sema::CheckBuiltinFunctionCall.

bob80905 added a comment.EditedOct 4 2022, 1:04 PM

Does these support scalable vector types from ARM SVE or RISC-V? I can't remember what the rest of the __builtin_elementwise do. I ask because the backends will probably crash for them.

I sat with @beanz and looked at the assembly code generated for the riscv64-unknown-elf target and the aarch64-apple-darwin target, with and without the sve target feature.
The compiler doesn't crash. The generated assembly looks good, there are 4 sinf calls, one generated for each element in the vector.

Here is the code I used to test the machine code output:

typedef float float4 __attribute__((ext_vector_type(4)));

void test_builtin_elementwise_sin(float f1, float f2, double d1, double d2, 
float4 vf1, float4 vf2)
{
  f2 = __builtin_elementwise_sin(f1);
  d2 = __builtin_elementwise_sin(d1);
  vf2 = __builtin_elementwise_sin(vf1);
}

f2 = builtin_elementwise_sin(f1); can be swapped for f2 = builtin_elementwise_cos(f1); to test the cos builtin,

Here is the code I used to test the machine code output:

typedef float float4 __attribute__((ext_vector_type(4)));

void test_builtin_elementwise_sin(float f1, float f2, double d1, double d2, 
float4 vf1, float4 vf2)
{
  f2 = __builtin_elementwise_sin(f1);
  d2 = __builtin_elementwise_sin(d1);
  vf2 = __builtin_elementwise_sin(vf1);
}

f2 = builtin_elementwise_sin(f1); can be swapped for f2 = builtin_elementwise_cos(f1); to test the cos builtin,

Not sure these will test scalable vector types.
Maybe something like vfloat32mf2_t or svfloat32_t?

bob80905 updated this revision to Diff 465235.Oct 4 2022, 5:24 PM
  • add sve tests to prove compiler doesn't crash
bob80905 updated this revision to Diff 465236.Oct 4 2022, 5:26 PM
  • add new line to end of new files
  • add sve tests to prove compiler doesn't crash

Thank you.

clang/docs/LanguageExtensions.rst
636

hypotenuse*

craig.topper added inline comments.Oct 4 2022, 10:27 PM
clang/docs/LanguageExtensions.rst
635

This list doesn't appear to be in an overall alphabetical order. It looks more like its grouped by similarity. add_sat/sub_sat are together for example.

scanon added inline comments.Oct 5 2022, 8:25 AM
clang/docs/LanguageExtensions.rst
636

As long as we're tweaking descriptions, please just call these "cosine" and "sine" instead of a cumbersome ratio description. "The cosine of x interpreted as an angle in radians" or similar.

bob80905 updated this revision to Diff 465446.Oct 5 2022, 10:09 AM
  • shorten builtin description, group sin and cos
craig.topper added inline comments.Oct 12 2022, 10:52 AM
clang/docs/ReleaseNotes.rst
760

Drop "llvm" from this sentence.

bob80905 updated this revision to Diff 467201.Oct 12 2022, 11:05 AM
  • drop llvm from release notes description.

Ping, could someone please take a look?

craig.topper accepted this revision.Thu, Nov 10, 9:35 AM

LGTM to me with those formatting fixes.

clang/test/CodeGen/builtins-elementwise-math.c
337

Line these up with the start of the arguments on the previous line.

385

Same

This revision is now accepted and ready to land.Thu, Nov 10, 9:35 AM
bob80905 updated this revision to Diff 474570.Thu, Nov 10, 10:16 AM
  • make format change
python3kgae added inline comments.Thu, Nov 10, 12:30 PM
clang/test/Sema/aarch64-sve-vector-trig-ops.c
5

Need limit to aarch64 enabled with
// REQUIRES: aarch64-registered-target

clang/test/Sema/riscv-sve-vector-trig-ops.c
5

// REQUIRES: riscv-registered-target

fhahn accepted this revision.Thu, Nov 10, 2:45 PM

LGTM with the suggested changes, thanks!

bob80905 updated this revision to Diff 474628.Thu, Nov 10, 4:16 PM
  • add dependency on architecture before testing
This revision was automatically updated to reflect the committed changes.