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

[Chad Rosier via Phabricator via llvm-commits]

mcrosier added a comment.

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



================
Comment at: lib/Target/AArch64/AArch64TargetTransformInfo.cpp:471
+  if (ST->isMisaligned128StoreSlow() && Opcode == Instruction::Store &&
+      Src->isVectorTy() && Alignment != 16 &&
       Src->getVectorElementType()->isIntegerTy(64)) {
----------------
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.


================
Comment at: test/Analysis/CostModel/AArch64/store.ll:2
+; RUN: opt < %s  -cost-model -analyze -mtriple=arm64-apple-ios -mattr=slow-misaligned-128store | FileCheck %s --check-prefix=SLOW_MISALIGNED_128_STORE
+; RUN: opt < %s  -cost-model -analyze -mtriple=arm64-apple-ios | FileCheck %s
+
----------------
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.


================
Comment at: test/Analysis/CostModel/AArch64/store.ll:6
 ; CHECK-LABEL: store
+; SLOW_MISALIGNED_128_STORE-LABEL: store
 define void @store() {
----------------
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.


https://reviews.llvm.org/D27677