This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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

Event Timeline

amyk created this revision.Jul 9 2020, 11:55 AM
amyk marked 2 inline comments as done.Jul 9 2020, 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.Jul 9 2020, 12:19 PM

Updated for clang format changes.

lei added inline comments.Jul 9 2020, 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.Jul 9 2020, 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.Jul 9 2020, 2:42 PM

Address review comments

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

Fix assignment of variable.

amyk updated this revision to Diff 278019.Jul 14 2020, 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.Jul 16 2020, 9:24 AM
kamaub added a subscriber: kamaub.

LGTM

This revision is now accepted and ready to land.Jul 16 2020, 9:24 AM
nemanjai requested changes to this revision.Jul 16 2020, 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.Jul 16 2020, 11:01 AM
amyk edited the summary of this revision. (Show Details)Jul 17 2020, 3:15 PM
amyk updated this revision to Diff 287142.Aug 21 2020, 6:42 PM

Address review comments:

  • Further consolidate the custom codegen of the two builtins
  • Add SemaChecking for if the third argument is a constant, if the third argument is in range and if the second argument is the same type as the element type of the first argument
  • Add extra test to test the semantic checks that were added
amyk updated this revision to Diff 287444.Aug 24 2020, 10:35 AM

Update to address clang-format.

amyk added a comment.Sep 14 2020, 7:03 AM

@nemanjai Could you please take another look to see if I have addressed your comments?

nemanjai added inline comments.Sep 17 2020, 7:12 PM
clang/lib/CodeGen/CGBuiltin.cpp
14285

// The input to vec_replace_elt is an element index, not a byte index.

14295

This is too vague.
// If the input vector is a float type, bitcast the inputs to integers.

14296

This seems to be duplicated in both blocks. Can we not just do something like
if (!Ops[1]->getType()->isIntegerTy(ArgWidth))? Then inside we can use the ternary operator to select between Int32Ty and Int64Ty if necessary.
Then we only need one of these bitcast blocks just before we emit the call.

14309

More specific comment please - just as above.

14318

s/resultant/result

clang/lib/Sema/SemaChecking.cpp
3196 ↗(On Diff #287444)

I am very surprised that this doesn't exist already but it seems more useful to have a static function in this file along the lines of:
static bool isEltOfVectorTy(QualType VectorTy, QualType EltTy)

That would do the obvious check.

3245 ↗(On Diff #287444)

I don't think the if statements add to readability. I think this should just be a single return statement and the range should be selected by a ternary op.
Something like:

unsigned Width = Context.getIntWidth(TheCall->getArg(1)->getType());
QualType VecTy = TheCall->getArg(0)->getType();
QualType EltTy = TheCall->getArg(1)->getType();
return SemaBuiltinConstantArgRange(TheCall, 2, 0, Width == 32 ? 12 : 8) ||
  isEltOfVectorTy(VecTy, EltTy);
nemanjai requested changes to this revision.Sep 17 2020, 7:32 PM

Just marking this not ready to keep my queue clean until the comments are addressed.

This revision now requires changes to proceed.Sep 17 2020, 7:32 PM
amyk updated this revision to Diff 293313.Sep 21 2020, 7:10 PM

Address Nemanja's review comments:

  • More specific comments when bitcasting the inputs
  • Pull out conditions to bitcast the input, use ternary op depending if the input is 32 or 64-bits
  • Create new static function to check if a given type is the same type as a vector element
amyk marked 10 inline comments as done.Sep 21 2020, 7:11 PM
amyk added a comment.Sep 22 2020, 8:00 AM

@nemanjai Would you please take another look to see if I have addressed your comments when you get a chance? Thanks.

nemanjai added inline comments.Sep 23 2020, 9:10 AM
clang/lib/Sema/SemaChecking.cpp
3165 ↗(On Diff #293313)

I think this should actually take a vector type and a scalar type. Then check that the scalar type is the same as the element type of the vector. The way this is implemented, a more apt name would be something like checkSameTypes().

amyk updated this revision to Diff 293779.Sep 23 2020, 9:56 AM
  • Updated the isEltOfVectorTy() to the correct semantics; making it take in a vector type and then getting the element type within the function.
amyk marked 2 inline comments as done.Sep 23 2020, 10:34 AM
nemanjai accepted this revision.Sep 23 2020, 11:14 AM

LGTM. Thanks for your patience and for addressing all the comments.

This revision is now accepted and ready to land.Sep 23 2020, 11:14 AM
This revision was landed with ongoing or failed builds.Sep 23 2020, 8:55 PM
This revision was automatically updated to reflect the committed changes.