This is an archive of the discontinued LLVM Phabricator instance.

[sve][acle] Add some C intrinsics for brain float types.
ClosedPublic

Authored by fpetrogalli on Jun 23 2020, 8:04 PM.

Details

Summary

The following intrinsics has been added:

svuint16_t svcnt[_bf16]_m(svuint16_t inactive, svbool_t pg, svbfloat16_t op)
svuint16_t svcnt[_bf16]_x(svbool_t pg, svbfloat16_t op)
svuint16_t svcnt[_bf16]_z(svbool_t pg, svbfloat16_t op)

svbfloat16_t svtbl[_bf16](svbfloat16_t data, svuint16_t indices)

svbfloat16_t svtbl2[_bf16](svbfloat16x2_t data, svuint16_t indices)

svbfloat16_t svtbx[_bf16](svbfloat16_t fallback, svbfloat16_t data, svuint16_t indices)

Diff Detail

Event Timeline

fpetrogalli created this revision.Jun 23 2020, 8:04 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 23 2020, 8:05 PM
c-rhodes added inline comments.Jun 24 2020, 8:45 AM
clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_tbl2-bfloat.c
5–6

Missing a test that checks for warning/error if __ARM_FEATURE_SVE_BF16 isn't defined, these two run lines are checking that for sve2 where we get an implicit declaration warning. See https://reviews.llvm.org/D82399#change-gND08cruJN2Z for an example

clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_tbx-bfloat.c
5–6

same here

llvm/lib/Target/AArch64/SVEInstrFormats.td
1024 ↗(On Diff #272902)

pattern needs to be predicated on +bf16. I suspect you might hit the same issue I did here: https://reviews.llvm.org/D82182#inline-758348

I found setting the predicate in the multiclass like that doesn't work as it's nested and there's an outer predicate (where the instruction is defined in AArch64SVEInstrInfo.td). For splice I found the following works:

let Predicates = [HasBF16, HasSVE] in {
  def : SVE_3_Op_Pat<nxv8bf16, int_aarch64_sve_splice, nxv8i1, nxv8bf16, nxv8bf16, SPLICE_ZPZ_H>;
}

by defining the pattern guarded on +bf16 in AArch64SVEInstrInfo.td .

1058 ↗(On Diff #272902)

as above, needs to be predicated on +bf16

1060 ↗(On Diff #272902)

nit: space

1108 ↗(On Diff #272902)

needs to be predicated on +bf16

3698 ↗(On Diff #272902)

needs to be predicated on +bf16

llvm/test/CodeGen/AArch64/sve-intrinsics-counting-bits.ll
1

need to add +bf16 to flags

153–154

nit: add space for alignment

llvm/test/CodeGen/AArch64/sve2-intrinsics-perm-tb.ll
1

need to add +bf16 to flags

fpetrogalli marked 11 inline comments as done.Jun 24 2020, 9:02 PM
fpetrogalli added inline comments.
llvm/test/CodeGen/AArch64/sve-intrinsics-counting-bits.ll
1

Instead of passing it as a command line argument, I have added an attribute to the functions that test bfloat, to keep it specific only for those functions.

fpetrogalli marked an inline comment as done.

Add predicate to the patterns in the backend.

fpetrogalli marked an inline comment as done.Jun 24 2020, 9:06 PM
fpetrogalli added inline comments.
clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_tbl2-bfloat.c
8

I could do with an extra pair of eyes here: I can't figure out why the warning raised by this run is not detected by the overload-bf16-warning below... (Same for the same line I have added in the test for tbx).

c-rhodes added inline comments.Jun 25 2020, 3:50 AM
clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_tbl2-bfloat.c
8

Ah, it works in the example I linked because whilerw / whilewr uses the scalar bfloat16_t, whereas this is using sizeless type which is predicated on -D__ARM_FEATURE_SVE_BF16 so we get:

error: 'error' diagnostics seen but not expected:
  File /home/culrho01/llvm-project/clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_tbl2-bfloat.c Line 18: unknown type name 'svbfloat16_t'; did you mean 'svfloat16_t'?
  File /home/culrho01/llvm-project/clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_tbl2-bfloat.c Line 18: unknown type name 'svbfloat16x2_t'; did you mean 'svfloat16x2_t'?

I'm not sure if/how we can test this for the overloaded form

c-rhodes accepted this revision.Jun 25 2020, 8:47 AM
c-rhodes added inline comments.
clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_tbl2-bfloat.c
8

I'm not sure if what I suggested makes sense - trying to do what we've done in the sve2 acle tests where we expect an implicit declaration warning for overloaded/non-overloaded intrinsics if the sve2 feature isn't enabled. I guess it's different for BF16 as the types are guarded on the feature macro in the ACLE, for whatever reason we get the same warning for the non-overloaded intrinsics but an error for the overloaded ones. I think we can be pretty confident +bf16 is required as the test will fail otherwise, but it's tricky trying to isolate an error implying the macro is missing on the intrinsic. FWIW we don't test this for SVE either, I think we can skip this test for the overloaded form, may as well keep the non-overloaded one in if it works.

This revision is now accepted and ready to land.Jun 25 2020, 8:47 AM

Removed the run lines that didn't work, as described in https://reviews.llvm.org/D82429#inline-759371

fpetrogalli marked 2 inline comments as done.Jun 25 2020, 9:27 AM
fpetrogalli added inline comments.
clang/test/CodeGen/aarch64-sve2-intrinsics/acle_sve2_tbl2-bfloat.c
8

Agree. I have removed the overload tests for the warning.

This revision was automatically updated to reflect the committed changes.
fpetrogalli marked an inline comment as done.