This is an archive of the discontinued LLVM Phabricator instance.

[X86] Remove __builtin_ia32_pabs intrinsics and use generic __builtin_elementwise_abs
ClosedPublic

Authored by RKSimon on Jan 20 2022, 8:23 AM.

Details

Summary

D111986 added the generic __builtin_elementwise_abs() intrinsic with the same integer absolute behaviour as the SSE/AVX instructions (abs(INT__MIN) == INT_MIN)

This patch removes the __builtin_ia32_pabs* intrinsics and just uses __builtin_elementwise_abs - the existing tests see no changes:

__m256i test_mm256_abs_epi8(__m256i a) {
  // CHECK-LABEL: test_mm256_abs_epi8
  // CHECK: [[ABS:%.*]] = call <32 x i8> @llvm.abs.v32i8(<32 x i8> %{{.*}}, i1 false)
  return _mm256_abs_epi8(a);
}

This requires us to add a __v64qs explicitly signed char vector type (we already have __v16qs and __v32qs).

Diff Detail

Event Timeline

RKSimon requested review of this revision.Jan 20 2022, 8:23 AM
RKSimon created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 8:23 AM
craig.topper added a comment.EditedJan 20 2022, 9:12 AM

Just to confirm since I can't see the test CHECK lines. builtin_elementwise_abs is not undefined for INT_MIN?

Just to confirm since I can't see the test CHECK lines. builtin_elementwise_abs is not undefined for INT_MIN?

That's why I added a copy of the check (from llvm-project\clang\test\CodeGen\X86\avx2-builtins.c) in the summary - the 'is_int_min_poison' flag in the abs intrinsic call is still false

Just to confirm since I can't see the test CHECK lines. builtin_elementwise_abs is not undefined for INT_MIN?

That's why I added a copy of the check (from llvm-project\clang\test\CodeGen\X86\avx2-builtins.c) in the summary - the 'is_int_min_poison' flag in the abs intrinsic call is still false

I shouldn't try to read patches too early in the morning I guess. Thanks.

This revision is now accepted and ready to land.Jan 20 2022, 11:37 AM
pengfei accepted this revision.Jan 20 2022, 5:04 PM
pengfei added inline comments.
clang/lib/Headers/avx512fintrin.h
31

Do we need to declare explicit signed?

craig.topper added inline comments.Jan 20 2022, 5:09 PM
clang/lib/Headers/avx512fintrin.h
31

The signedness of char is platform specific.

pengfei added inline comments.Jan 20 2022, 5:14 PM
clang/lib/Headers/avx512fintrin.h
31

Got it, thank you!

This revision was landed with ongoing or failed builds.Jan 21 2022, 3:59 AM
This revision was automatically updated to reflect the committed changes.

Quick update: we're hitting an issue with the libc-x86_64-debian-fullbuild-dbg and asan build bots which is using the wrong headers and breaks when the __builtin_ia32_pabs* builtins are removed. Once that has been addressed I'll recommit.

RKSimon edited the summary of this revision. (Show Details)Jan 21 2022, 9:54 AM