[AArch64] Guard Misaligned 128-bit store penalty by subtarget feature

[mssimpso]

Summary

This patch checks that the SlowMisaligned128Store subtarget feature is set when penalizing misaligned stores in getMemoryOpCost TTI function.

[MatzeB]

Isn't this the same as "FeatureSlowMisaligned128Store"?

lib/Target/AArch64/AArch64Subtarget.cpp

74 ↗

(On Diff #81093)

Use target a target feature (or rather reuse the existing one) and let tablegen deal with a boolean flag instead of manually setting it here. (This switch is only needed for integer values because the tablegen logic only deals with booleans at the moment).

[mssimpso]

In D27677#620116, @MatzeB wrote:

Isn't this the same as "FeatureSlowMisaligned128Store"?

I wasn't aware of this, thanks! It looks like Cyclone is the only subtarget that sets FeatureSlowMisaligned128Store at the moment. To preserve existing behavior for all subtargets (except Kryo and Falkor), should we also set this feature for the others?

[MatzeB]

In D27677#620131, @mssimpso wrote:

In D27677#620116, @MatzeB wrote:

Isn't this the same as "FeatureSlowMisaligned128Store"?

I wasn't aware of this, thanks! It looks like Cyclone is the only subtarget that sets FeatureSlowMisaligned128Store at the moment. To preserve existing behavior for all subtargets (except Kryo and Falkor), should we also set this feature for the others?

If you set it for most of the other CPUs then you will change behavior of AArch64TargetLowering::allowsMisalignedMemoryAccesses() and that performStoreCombine() thing. It may very well be one of the many artifacts showing that the aarch64 target was initially designed for cyclone, however someone working on the other ARM chips should comment on that.

[mssimpso]

Right. I'll update the patch to just use isMisaligned128StoreSlow() in getMemoryOpCost, and add some more folks to the review. Thanks!

[mssimpso]

Addressed Matthias's comments. Thanks!

• We now use the existing FeatureSlowMisaligned128Store instead of adding a new feature.
• I added James and Sebastian to the review for opinions about non-Cyclone CPUs.

[evandro]

This feature is rather a subset of FeatureAvoidQuadLdStPairs, used in AArch64InstrInfo::isCandidateToMergeOrPair(), which effectively avoids any 128 load or store, aligned or misaligned.

I actually wonder if, for uniformity's sake, FeatureAvoidQuadLdStPairs should be renamed FeatureSlow256LoadStore. Thoughts?

[evandro]

BTW, is FeatureSlowMisaligned128Store really meant for v2i64 only and for no other 128-bit vector type?

[mssimpso]

In D27677#620254, @evandro wrote:

I actually wonder if, for uniformity's sake, FeatureAvoidQuadLdStPairs should be renamed FeatureSlow256LoadStore. Thoughts?

We can certainly discuss renaming that feature, but I think we should probably do so in a separate review/thread.

BTW, is FeatureSlowMisaligned128Store really meant for v2i64 only and for no other 128-bit vector type?

It doesn't look like the feature is limited to v2i64. The fact that the TTI function explicitly checks for an integer type seems wrong to me. But I think Matthias is probably right in that this heuristic was originally intended for Cyclone. It looks fairly clear to me that the heuristic should be guarded by FeatureSlowMisaligned128Store. This will preserve the existing functionality for Cyclone, but will change the other subtargets. Would you have any concerns for Exynos were this the case?

[evandro]

In D27677#621093, @mssimpso wrote:

It doesn't look like the feature is limited to v2i64. The fact that the TTI function explicitly checks for an integer type seems wrong to me. But I think Matthias is probably right in that this heuristic was originally intended for Cyclone. It looks fairly clear to me that the heuristic should be guarded by FeatureSlowMisaligned128Store. This will preserve the existing functionality for Cyclone, but will change the other subtargets. Would you have any concerns for Exynos were this the case?

Since this patch qualifies this transformation with FeatureSlowMisaligned128Store, I'm comfortable with verifying if Exynos needs this feature or not in a follow up patch, if needed.

[mssimpso]

In D27677#621103, @evandro wrote:

Since this patch qualifies this transformation with FeatureSlowMisaligned128Store, I'm comfortable with verifying if Exynos needs this feature or not in a follow up patch, if needed.

Great. Thanks very much for the feedback, Evandro.

[mssimpso]

Updated test case comment.

[mcrosier]

LGTM! Internally, the performance results for SPEC200X looked good with a few 1-3% gains. One minor regression reported on an internal benchmark.

lib/Target/AArch64/AArch64TargetTransformInfo.cpp

471 ↗

(On Diff #81442)

A comment to the Cyclone owners and not this particular patch:

Is it possible for the Alignment to be > 16? If so, I'm wondering if the alignment check should be 'Alignment < 16'? Just something I saw in passing.

test/Analysis/CostModel/AArch64/store.ll

2 ↗

(On Diff #81442)

Perhaps put this RUN line first, since it's the default behavior. We might also consider changing the arm64 to aarch64 on both RUN lines.

6 ↗

(On Diff #81442)

I'm not a fan of the function name matching an IR keyword.

store -> @store(

..or we might consider a different name for the test.

[mssimpso]

Addressed Chad's comments. Thanks!

llvm/trunk/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

int AArch64TTIImpl::getCmpSelInstrCost(unsigned Opcode, Type *ValTy,
   }
   return BaseT::getCmpSelInstrCost(Opcode, ValTy, CondTy);
 }
 
 int AArch64TTIImpl::getMemoryOpCost(unsigned Opcode, Type *Src,
                                     unsigned Alignment, unsigned AddressSpace) {
   std::pair<int, MVT> LT = TLI->getTypeLegalizationCost(DL, Src);
 
-  if (Opcode == Instruction::Store && Src->isVectorTy() && Alignment != 16 &&
+  if (ST->isMisaligned128StoreSlow() && Opcode == Instruction::Store &&
+      Src->isVectorTy() && Alignment != 16 &&
       Src->getVectorElementType()->isIntegerTy(64)) {
     // Unaligned stores are extremely inefficient. We don't split
     // unaligned v2i64 stores because the negative impact that has shown in
     // practice on inlined memcpy code.
     // We make v2i64 stores expensive so that we will only vectorize if there
     // are 6 other instructions getting vectorized.
     int AmortizationCost = 6;
 

llvm/trunk/test/Analysis/CostModel/AArch64/store.ll

-; RUN: opt < %s  -cost-model -analyze -mtriple=arm64-apple-ios -mcpu=cyclone | FileCheck %s
+; RUN: opt < %s  -cost-model -analyze -mtriple=aarch64-apple-ios | FileCheck %s
+; RUN: opt < %s  -cost-model -analyze -mtriple=aarch64-apple-ios -mattr=slow-misaligned-128store | FileCheck %s --check-prefix=SLOW_MISALIGNED_128_STORE
+
 target datalayout = "e-p:32:32:32-i1:8:32-i8:8:32-i16:16:32-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:32:64-v128:32:128-a0:0:32-n32-S32"
-; CHECK-LABEL: store
-define void @store() {
-    ; Stores of <2 x i64> should be expensive because we don't split them and
-    ; and unaligned 16b stores have bad performance.
-    ; CHECK: cost of 12 {{.*}} store
+; CHECK-LABEL: getMemoryOpCost
+; SLOW_MISALIGNED_128_STORE-LABEL: getMemoryOpCost
+define void @getMemoryOpCost() {
+    ; If FeatureSlowMisaligned128Store is set, we penalize <2 x i64> stores. On
+    ; Cyclone, for example, such stores should be expensive because we don't
+    ; split them and misaligned 16b stores have bad performance.
+    ;
+    ; CHECK: cost of 1 {{.*}} store
+    ; SLOW_MISALIGNED_128_STORE: cost of 12 {{.*}} store
     store <2 x i64> undef, <2 x i64> * undef
 
     ; We scalarize the loads/stores because there is no vector register name for
     ; these types (they get extended to v.4h/v.2s).
     ; CHECK: cost of 16 {{.*}} store
     store <2 x i8> undef, <2 x i8> * undef
     ; CHECK: cost of 64 {{.*}} store
     store <4 x i8> undef, <4 x i8> * undef
     ; CHECK: cost of 16 {{.*}} load
     load <2 x i8> , <2 x i8> * undef
     ; CHECK: cost of 64 {{.*}} load
     load <4 x i8> , <4 x i8> * undef
 
     ret void
 }