[AArch64][SVE] Instcombine SVE LD1/ST1 to stock LLVM IR

[MattDevereau]

Summary

InstCombine AArch64 LD1/ST1 to llvm.masked.load/llvm.masked.store and LD1/ST1 to load/store when a ptrue all predicate pattern operand is present.

This allows existing IR optimizations such as dead-load removal to occur.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

718

Can we use a masked load for this scenario also? As in masked_load doesn't care how the mask was constructed.

My feeling is instCombineSVEMaskedLD1 and instCombineSVELD1 are called in the wrong order. It seems easier to always call instCombineSVEMaskedLD1 and then call instCombineSVELD1 for the case when

match(II.getOperand(0),
             m_Intrinsic<Intrinsic::aarch64_sve_ptrue>(m_ConstantInt<AArch64SVEPredPattern::all>()))

is true?

[peterwaller-arm]

I've had a brief look, some typographical comments.

For some reason the patch has failed to apply and the tests have failed to run in CI, perhaps you could try a rebase and rerun arc diff to see if that fixes it?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

701

Conventionally, I think this would be called VecTy rather than VecOp.

702

I've seen more uses of Ptr than "Pointer".

724

The name VScale brings up an association with "The runtime value of VScale", and this is not a pointer to it, so I think a name more in keeping with the codebase would be something like VecPtr.

733

Inconsistent use of auto (Value* was used above, which I think the coding guide would prefer).

734

You've named getOperand(0) and getOperand(2) here, I think for consistency/completeness you may as well name the final operand, even though it's only used once.

[MattDevereau]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

718

Using the match you suggested and always calling instCombineSVEMaskedLD1 would end up instCombining and erasing aarch64.sve.convert.from.svbool, predicates I think? Is this ok?

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

707–709

I also just noticed you're not specifying the Passthru parameter for CreateMaskedLoad. This is very important because the ld1 intrinsics are defined to zero the inactive lanes and so you need to pass in an explicit zero vector here. You'll see similar in instCombineLD1GatherIndex.

718

I don't understand your reasoning here.

aarch64.sve.convert.from.svbool is just an intrinsic that produces a predicate. If that is the predicate operand to the ld1 then it should end up being the mask operand of the masked_load.

For the case where the predicate operand is specifically ptrue 31 we can drop the predicate operand and call Builder.CreateLoad with all other cases passing all the operands to Builder.CreateMaskedLoad. Perhaps that means we don't even need instCombineSVELD1.

[MattDevereau]

Add CreateMaskedLoad passthru parameter
Renamed most variable names to fit with LLVM code convention more
Reworked some function/match logic

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

718

Nevermind, I realised I worded my question incorrectly and then even when worded correctly it made no sense either. I've changed the match.

[peterwaller-arm]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

731

VecPtrTy -- the Ty conventionally goes at the end.

783

s/VecTyPtr/VecPtrTy/

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

728–738

Does this function and instCombineSVEST1 offer any real value? I cannot see them having other uses and the implementation is largely already available in the caller. I think it would be cleaner to just have the

{
  LoadInst *Load = Builder.CreateLoad(II.getType(), VecPtr);
  return IC.replaceInstUsesWith(II, Load);
}

part inside instCombineSVEMaskedLD1.

769

Is this necessary?

788

Is this necessary?

[peterwaller-arm]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

769

Adding (void) was my guidance, which on reflection I think was in error, thanks for pointing it out.

788

Adding (void) was my guidance, which on reflection I think was in error, thanks for pointing it out.

[DavidTruby]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

732

Nit: I think the default template arguments should just be picked without the <>

750

765

769

If you wanted to make sure you didn't get unused return value warnings you can use LLVM_ATTRIBUTE_UNUSED to be a bit more clear than the void cast.

[MattDevereau]

Regenerated broken tests
Combined instCombineSVEMaskedLD1 and instCombineSVELD1
Combined instCombineSVEMaskedST1 and instCombineSVEST1

[MattDevereau]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

732

I'm getting compiler errors when omitting <>

750

I'm getting compiler errors when omitting <>

765

I'm getting compiler errors when omitting <>

[MattDevereau]

Run clang-format

[peterwaller-arm]

clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_st1w.c

235 ↗

(On Diff #387173)

Unfortunately it looks like these were not autogenerated, they used to use CHECK-DAG. This results in more changes than we expect. My thinking is that these tests are intended to be autogenerated, so we should make a clean patch which autogenerates the tests and then reapply your work on top of that.

[peterwaller-arm]

It looks like some auto-generation lines were added in 91eda9c30f33da6ec6da70b59a5f5da6c6397039, but the tests using CHECK-DAG were not actually auto-generated. It seems we could either cleanly auto-generate them, or we should copy the existing CHECK-DAG patterns. It seems that what is currently committed is a little wrong, if only by having a 'these tests are auto-generated' header even though they are not auto-generated.

[peterwaller-arm]

My current thinking is that the route forward is to update the tests without update test checks, which affects only the st1{b,h,w} files, and possibly to remove the false header implying these files were autogenerated.

[paulwalker-arm]

There's an issue with the a Value* being named VecPtrTy but otherwise this looks good to me.

I'll leave it up to you to decide whether it's worth breaking out the usage of update_cc_test_checks.py into a separate patch. Normally this is a good thing but given the patch is ready to go and I imagine you wouldn't submit the "use update_cc_test_checks.py" patch for review I'm not sure it's worth the effort.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

736

The name here is wrong as this is not a type. My guess is you meant VecPtr?

758

As above, I suspect this name is not what you intended.

[MattDevereau]

Renamed VecPtrTy to VecPtr in both instCombineSVEST1 and instCombineSVELD1
Moved irrelevant changes from regenerating tests to commit 485c193aa12addea13a0db12f4c6bc6252244319

[MattDevereau]

This caused buildbot failures which failed on the bfloat tests. Pushed commit 83727f27719d3f319f746b473ce09be7e1d99b32 to fix the issue

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

static Optional<Instruction *> instCombineSVEPTest(InstCombiner &IC,
   }
 
   return None;
 }
 
 static Optional<Instruction *> instCombineSVEVectorFMLA(InstCombiner &IC,
                                                         IntrinsicInst &II) {
   // fold (fadd p a (fmul p b c)) -> (fma p a b c)
   Value *P = II.getOperand(0);
   Value *A = II.getOperand(1);
   auto FMul = II.getOperand(2);
   Value *B, *C;
   if (!match(FMul, m_Intrinsic<Intrinsic::aarch64_sve_fmul>(
                        m_Specific(P), m_Value(B), m_Value(C))))
     return None;
 
   if (!FMul->hasOneUse())
     return None;
 
   llvm::FastMathFlags FAddFlags = II.getFastMathFlags();
   // Stop the combine when the flags on the inputs differ in case dropping flags
   // would lead to us missing out on more beneficial optimizations.
   if (FAddFlags != cast<CallInst>(FMul)->getFastMathFlags())
     return None;
   if (!FAddFlags.allowContract())
     return None;
 
   IRBuilder<> Builder(II.getContext());
   Builder.SetInsertPoint(&II);
   auto FMLA = Builder.CreateIntrinsic(Intrinsic::aarch64_sve_fmla,
                                       {II.getType()}, {P, A, B, C}, &II);
   FMLA->setFastMathFlags(FAddFlags);
   return IC.replaceInstUsesWith(II, FMLA);
 }
 
+static Optional<Instruction *>
+instCombineSVELD1(InstCombiner &IC, IntrinsicInst &II, const DataLayout &DL) {
+  Value *PtrOp = II.getOperand(1);
+  Type *VecTyPtr = II.getType()->getPointerTo();
+  IRBuilder<> Builder(II.getContext());
+  Builder.SetInsertPoint(&II);
+
+  Value *VecPtr = Builder.CreateBitCast(PtrOp, VecTyPtr);
+  LoadInst *Load = Builder.CreateLoad(II.getType(), VecPtr);
+  return IC.replaceInstUsesWith(II, Load);
+}
+
+static Optional<Instruction *> instCombineSVEMaskedLD1(InstCombiner &IC,
+                                                       IntrinsicInst &II,
+                                                       const DataLayout &DL) {
+  Value *Pred = II.getOperand(0);
+  if (match(Pred, m_Intrinsic<Intrinsic::aarch64_sve_ptrue>(
+                      m_ConstantInt<AArch64SVEPredPattern::all>())))
+    return instCombineSVELD1(IC, II, DL);
+
+  Type *VecTy = II.getType();
+  Value *PtrOp = II.getOperand(1);
+  IRBuilder<> Builder(II.getContext());
+  Builder.SetInsertPoint(&II);
+
+  Value *VecPtr = Builder.CreateBitCast(PtrOp, VecTy->getPointerTo());
+  CallInst *MaskedLoad =
+      Builder.CreateMaskedLoad(VecTy, VecPtr, PtrOp->getPointerAlignment(DL),
+                               Pred, ConstantAggregateZero::get(VecTy));
+  return IC.replaceInstUsesWith(II, MaskedLoad);
+}
+
+static Optional<Instruction *>
+instCombineSVEST1(InstCombiner &IC, IntrinsicInst &II, const DataLayout &DL) {
+  Value *VecOp = II.getOperand(0);
+  Value *PtrOp = II.getOperand(2);
+  Type *VecTyPtr = VecOp->getType()->getPointerTo();
+  IRBuilder<> Builder(II.getContext());
+  Builder.SetInsertPoint(&II);
+
+  Value *VecPtr = Builder.CreateBitCast(PtrOp, VecTyPtr);
+  (void)Builder.CreateStore(VecOp, VecPtr);
+  return IC.eraseInstFromFunction(II);
+}
+
+static Optional<Instruction *> instCombineSVEMaskedST1(InstCombiner &IC,
+                                                       IntrinsicInst &II,
+                                                       const DataLayout &DL) {
+  Value *Pred = II.getOperand(1);
+  if (match(Pred, m_Intrinsic<Intrinsic::aarch64_sve_ptrue>(
+                      m_ConstantInt<AArch64SVEPredPattern::all>())))
+    return instCombineSVEST1(IC, II, DL);
+
+  Value *VecOp = II.getOperand(0);
+  Value *PtrOp = II.getOperand(2);
+  Type *VecTyPtr = VecOp->getType()->getPointerTo();
+  IRBuilder<> Builder(II.getContext());
+  Builder.SetInsertPoint(&II);
+
+  auto VecPtr = Builder.CreateBitCast(PtrOp, VecTyPtr);
+  (void)Builder.CreateMaskedStore(VecOp, VecPtr, PtrOp->getPointerAlignment(DL),
+                                  Pred);
+  return IC.eraseInstFromFunction(II);
+}
+
 static Instruction::BinaryOps intrinsicIDToBinOpCode(unsigned Intrinsic) {
   switch (Intrinsic) {
   case Intrinsic::aarch64_sve_fmul:
     return Instruction::BinaryOps::FMul;
   case Intrinsic::aarch64_sve_fadd:
     return Instruction::BinaryOps::FAdd;
   case Intrinsic::aarch64_sve_fsub:
     return Instruction::BinaryOps::FSub;
case Intrinsic::aarch64_sve_tuple_get:
     return instCombineSVETupleGet(IC, II);
   case Intrinsic::aarch64_sve_zip1:
   case Intrinsic::aarch64_sve_zip2:
     return instCombineSVEZip(IC, II);
   case Intrinsic::aarch64_sve_ld1_gather_index:
     return instCombineLD1GatherIndex(IC, II);
   case Intrinsic::aarch64_sve_st1_scatter_index:
     return instCombineST1ScatterIndex(IC, II);
+  case Intrinsic::aarch64_sve_ld1:
+    return instCombineSVEMaskedLD1(IC, II, DL);
+  case Intrinsic::aarch64_sve_st1:
+    return instCombineSVEMaskedST1(IC, II, DL);
   }
 
   return None;
 }
 
 bool AArch64TTIImpl::isWideningInstruction(Type *DstTy, unsigned Opcode,
                                            ArrayRef<const Value *> Args) {
 

llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-loadstore.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instcombine < %s | FileCheck %s
+
+target triple = "aarch64-unknown-linux-gnu"
+
+define <vscale x 4 x i32> @combine_ld1(i32* %ptr) #0 {
+; CHECK-LABEL: @combine_ld1(
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i32* [[PTR:%.*]] to <vscale x 4 x i32>*
+; CHECK-NEXT:    [[TMP2:%.*]] = load <vscale x 4 x i32>, <vscale x 4 x i32>* [[TMP1]], align 16
+; CHECK-NEXT:    ret <vscale x 4 x i32> [[TMP2]]
+;
+  %1 = tail call <vscale x 4 x i1> @llvm.aarch64.sve.ptrue.nxv4i1(i32 31)
+  %2 = call <vscale x 4 x i32> @llvm.aarch64.sve.ld1.nxv4i32(<vscale x 4 x i1> %1, i32* %ptr)
+  ret <vscale x 4 x i32> %2
+}
+
+define <vscale x 4 x i32> @combine_ld1_masked(i32* %ptr) #0 {
+; CHECK-LABEL: @combine_ld1_masked(
+; CHECK-NEXT:    [[TMP1:%.*]] = tail call <vscale x 4 x i1> @llvm.aarch64.sve.ptrue.nxv4i1(i32 16)
+; CHECK-NEXT:    [[TMP2:%.*]] = bitcast i32* [[PTR:%.*]] to <vscale x 4 x i32>*
+; CHECK-NEXT:    [[TMP3:%.*]] = call <vscale x 4 x i32> @llvm.masked.load.nxv4i32.p0nxv4i32(<vscale x 4 x i32>* [[TMP2]], i32 1, <vscale x 4 x i1> [[TMP1]], <vscale x 4 x i32> zeroinitializer)
+; CHECK-NEXT:    ret <vscale x 4 x i32> [[TMP3]]
+;
+  %1 = tail call <vscale x 4 x i1> @llvm.aarch64.sve.ptrue.nxv4i1(i32 16)
+  %2 = call <vscale x 4 x i32> @llvm.aarch64.sve.ld1.nxv4i32(<vscale x 4 x i1> %1, i32* %ptr)
+  ret <vscale x 4 x i32> %2
+}
+
+define void @combine_st1(<vscale x 4 x i32> %vec, i32* %ptr) #0 {
+; CHECK-LABEL: @combine_st1(
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i32* [[PTR:%.*]] to <vscale x 4 x i32>*
+; CHECK-NEXT:    store <vscale x 4 x i32> [[VEC:%.*]], <vscale x 4 x i32>* [[TMP1]], align 16
+; CHECK-NEXT:    ret void
+;
+  %1 = tail call <vscale x 4 x i1> @llvm.aarch64.sve.ptrue.nxv4i1(i32 31)
+  call void @llvm.aarch64.sve.st1.nxv4i32(<vscale x 4 x i32> %vec, <vscale x 4 x i1> %1, i32* %ptr)
+  ret void
+}
+
+define void @combine_st1_masked(<vscale x 4 x i32> %vec, i32* %ptr) #0 {
+; CHECK-LABEL: @combine_st1_masked(
+; CHECK-NEXT:    [[TMP1:%.*]] = tail call <vscale x 4 x i1> @llvm.aarch64.sve.ptrue.nxv4i1(i32 16)
+; CHECK-NEXT:    [[TMP2:%.*]] = bitcast i32* [[PTR:%.*]] to <vscale x 4 x i32>*
+; CHECK-NEXT:    call void @llvm.masked.store.nxv4i32.p0nxv4i32(<vscale x 4 x i32> [[VEC:%.*]], <vscale x 4 x i32>* [[TMP2]], i32 1, <vscale x 4 x i1> [[TMP1]])
+; CHECK-NEXT:    ret void
+;
+  %1 = tail call <vscale x 4 x i1> @llvm.aarch64.sve.ptrue.nxv4i1(i32 16)
+  call void @llvm.aarch64.sve.st1.nxv4i32(<vscale x 4 x i32> %vec, <vscale x 4 x i1> %1, i32* %ptr)
+  ret void
+}
+
+declare void @llvm.aarch64.sve.st1.nxv4i32(<vscale x 4 x i32>, <vscale x 4 x i1>, i32*)
+declare <vscale x 4 x i1> @llvm.aarch64.sve.ptrue.nxv4i1(i32)
+declare <vscale x 16 x i1> @llvm.aarch64.sve.ptrue.nxv16i1(i32)
+declare <vscale x 4 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv4i1(<vscale x 16 x i1>)
+declare <vscale x 4 x i32> @llvm.aarch64.sve.ld1.nxv4i32(<vscale x 4 x i1>, i32*)
+
+attributes #0 = { "target-features"="+sve" }