[PATCH] D151870: [ARM] Increase cost of unaligned double and float loads

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs added a comment.

In D151870#4390024 <https://reviews.llvm.org/D151870#4390024>, @dmgreen wrote:

> Sounds good. Remember to upload with context!

Oh yeah! It's been a while since I last submitted a patch.

In D151870#4390194 <https://reviews.llvm.org/D151870#4390194>, @SjoerdMeijer wrote:

> Just nits, lgtm too.

Thanks :)

> One question about my favourite fp16 data type while we are at it. Does or can that benefit from some handling too here?

I haven't considered fp16 since it didn't come up when I noticed this issue, but I can certainly add checking for it.

> And one last question about this:
>
>> // Unaligned float loads introduce an extra store and a register move
>> // The codegen for these unaligned loads could be improved,
>
> Was just curious if this is visible in an existing test and if there's a FIXME/TODO as a comment.

I noticed this issue after implementing something downstream and it has been tough to make upstream clang make unaligned loads to act as a test case for this. If you can think of some C that produces unaligned loads then I'd gladly add it.



================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:1502
+  // represent what is currently produced
+  if (ST->hasFPRegs64() && Src->isDoubleTy() && Alignment && Alignment.valueOrOne() < 8)
+    return 6;
----------------
SjoerdMeijer wrote:
> Do you need to check Alignment (` .. && Alignment && ..`) when you use `valueOrOne()`?
I believe we do, since if the instruction doesn't have an alignment operand, it uses the type's natural alignment and won't have the extra stack instructions.


================
Comment at: llvm/test/Analysis/CostModel/ARM/load_store.ll:73
 ; CHECK-MVE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: store float undef, ptr undef, align 4
-; CHECK-MVE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: store double undef, ptr undef, align 4
+; CHECK-MVE-NEXT:  Cost Model: Found an estimated cost of 6 for instruction: store double undef, ptr undef, align 4
 ; CHECK-MVE-NEXT:  Cost Model: Found an estimated cost of 18 for instruction: store <2 x i8> undef, ptr undef, align 1
----------------
dmgreen wrote:
> It might be worth adding a test with an alignment of 8 to be sure the cost remains OK. And add an unaligned float test.
Good idea


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151870/new/

https://reviews.llvm.org/D151870