Page MenuHomePhabricator

[PowerPC][Power10] Implement custom codegen for the vec_replace_elt and vec_replace_unaligned builtins.
Needs RevisionPublic

Authored by amyk on Thu, Jul 9, 11:55 AM.

Details

Reviewers
power-llvm-team
nemanjai
lei
kamaub
Group Reviewers
Restricted Project
Summary

This patch implements custom codegen for the vec_replace_elt and vec_replace_unaligned builtins.

These builtins map to the @llvm.ppc.altivec.vinsw and @llvm.ppc.altivec.vinsd intrinsics depending on the arguments.
The main motivation for doing custom codegen for these intrinsics is because there are float and double versions of the
builtin. Normally, the converting the float to an integer would be done via fptoui in the IR. This is incorrect as fptoui
truncates the value and we must ensure the value is not truncated. Therefore, we provide custom codegen to utilize
bitcast instead as bitcasts do not truncate.

The original patch that implemented the front end done this adding unions to altivec.h (https://reviews.llvm.org/D82359) but
this patch uses custom codegen to use bitcast instead for the float conversion instead.

Depends on D83497.

Diff Detail

Unit TestsFailed

TimeTest
500 mslinux > MemorySanitizer-X86_64.MemorySanitizer-X86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -gline-tables-only -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/strxfrm.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/strxfrm.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/X86_64/Output/strxfrm.cpp.tmp
250 mslinux > MemorySanitizer-lld-X86_64.MemorySanitizer-lld-X86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=memory -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -m64 -fuse-ld=lld -gline-tables-only -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/msan/strxfrm.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/strxfrm.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/msan/lld-X86_64/Output/strxfrm.cpp.tmp
520 mslinux > SanitizerCommon-asan-x86_64-Linux.Linux::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=address -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/asan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/asan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
300 mslinux > SanitizerCommon-lsan-x86_64-Linux.Linux::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=leak -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/lsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/lsan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
460 mslinux > SanitizerCommon-msan-x86_64-Linux.Linux::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=memory -m64 -ldl -std=c++11 -O0 -g /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Linux/Output/protoent.cpp.tmp && /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Linux/Output/protoent.cpp.tmp 2>&1 | FileCheck /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/sanitizer_common/TestCases/Linux/protoent.cpp
View Full Test Results (7 Failed)

Event Timeline

amyk created this revision.Thu, Jul 9, 11:55 AM
amyk marked 2 inline comments as done.Thu, Jul 9, 12:03 PM
amyk added inline comments.
clang/include/clang/Basic/BuiltinsPPC.def
339

I originally intended to implement this like the xxpermdi builtin:

BUILTIN(__builtin_vsx_xxpermdi, "v.", "t")

to use v. but I am not able to declare these builtins as void. For now, they're more or less an arbitrary signature that would match vinsw.

clang/test/CodeGen/builtins-ppc-p10vector.c
606

I've utilized tests that were from Biplob's original patch (https://reviews.llvm.org/D82359), but added the bitcasts to the float/double cases.

amyk updated this revision to Diff 276804.Thu, Jul 9, 12:19 PM

Updated for clang format changes.

lei added inline comments.Thu, Jul 9, 12:40 PM
clang/lib/CodeGen/CGBuiltin.cpp
14273

Do you mean?

// The third argument of vec_replace_elt must be a compile time constant and will be emitted either
//  to the vinsw or vinsd instruction.
14289
ConstArg *= 4;
// Fix the constant according to endianess.
if (getTarget().isLittleEndian())
   ConstArg = 12 - ConstArg;
14320

What are the chances of reaching to the end of this if/else-if section and Call is null? ie getPrimitiveSizeInBits() != [32|64]
I feel like it would be better if we can structure it so that we are not doing all these nesting of ifs and just do returns within the diff if-conditions.

Have you tried to pull out the diff handling of 32/64bit arg and consolidating the code a bit?

amyk marked 3 inline comments as done.Thu, Jul 9, 2:41 PM
amyk added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
14273

Yes. Thank you - I will update the wording here and in the other builtin.

14320

Thanks - I realize that I should probably pull the Call out. I'll update this.
I've actually consolidated the code quite a bit already, but I'll see if I can make any further improvements on this.

amyk updated this revision to Diff 276844.Thu, Jul 9, 2:42 PM

Address review comments

  • update comments
  • pull out common code
amyk marked 2 inline comments as done.Thu, Jul 9, 2:42 PM
amyk updated this revision to Diff 276853.Thu, Jul 9, 3:27 PM

Fix assignment of variable.

amyk updated this revision to Diff 278019.Tue, Jul 14, 4:39 PM
amyk edited the summary of this revision. (Show Details)

Corrected the patch as it previously caused errors to the clang test case.

kamaub accepted this revision as: kamaub.Thu, Jul 16, 9:24 AM
kamaub added a subscriber: kamaub.

LGTM

This revision is now accepted and ready to land.Thu, Jul 16, 9:24 AM
nemanjai requested changes to this revision.Thu, Jul 16, 11:01 AM

The description includes ... however it is more preferable to use bitcast. It is not a question of preference but of correctness. The fp to int conversions truncate while bitcasts don't. The semantics of the builtins require that no truncation happen.

Also, please include checks in SemaChecking for:

  • Third argument being constant
  • Third argument being within range
  • Second argument having the same type as the element type of the first
clang/lib/CodeGen/CGBuiltin.cpp
14275

Where is the code that ensures this? There does not appear to be a Sema check to emit a meaningful message for this. We also need a test with a non-constant argument to show the message.

14278

I don't think we should be creating the declaration if we may not use it. Just initialize this to nullptr here and set it for each case.

14307

Please change this to a negative condition (i.e. if the type is not i64). Similarly in other similar conditions.

14319

Can we reorganize this as something like:

case PPC::BI__builtin_altivec_vec_replace_elt:
case PPC::BI__builtin_altivec_vec_replace_unaligned: {
  // Define variables that are needed
  unsigned ArgWidth = Ops[1]->getType()->getPrimitiveSizeInBits();
  if (BuiltinID == PPC::BI__builtin_altivec_vec_replace_elt)
    ConstArg *= ArgWidth / 8;
  assert((ArgWidth == 32 || ArgWidth == 64) && "Invalid argument width");
  if (ArgWidth == 32) {
    // set up what is needed for vinsw
  } else {
    // set up what is needed for vinsd
  }
  // Emit the call
  if (BuiltinID == PPC::BI__builtin_altivec_vec_replace_elt)
    // add the bitcast of the result
}
This revision now requires changes to proceed.Thu, Jul 16, 11:01 AM
amyk edited the summary of this revision. (Show Details)Fri, Jul 17, 3:15 PM