[PATCH] D63246: [X86][SSE] Prevent misaligned non-temporal vector load/store combines

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

In D63246#1544547 <https://reviews.llvm.org/D63246#1544547>, @lebedev.ri wrote:

> Also, judging by the LHS of the diffs, there isn't anything to split those loads/stores currently?


I'm adding more tests as I can but need to get correct allowsMisalignedMemoryAccesses handling in before I can improve combineLoad and combienStore - otherwise the existing load/store merge combines will fight against us (e.g. nt-load splitting fails horribly....).



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2133-2136
+    if (!!(Flags & MachineMemOperand::MOLoad) &&
+        (Align < 16 || !Subtarget.hasSSE41()))
+      return true;
+    return false;
----------------
lebedev.ri wrote:
> So this says that if this is a non-temporal load of a vector,
> either from a pointer that is aligned so little that we can't even make 128-bit aligned load from,
> or we do not even have SSE41 (so no aligned non-temporal vector loads at all),
> the underaligned loading is allowed, correct?
> This kinda looks backwards to me?
> I expected something like 
> ```
>   if (!!(Flags & MachineMemOperand::MONonTemporal) && VT.isVector()) {
>     return !!(Flags & MachineMemOperand::MOLoad) &&
>            Align >= VT.getSizeInBytes() && Subtarget.hasSSE41());
> }
> ```
What I have looks correct to me - we need to return true if the misaligned load is acceptable - so if align<16 or we don't have SSE41 - but I will pull out more of the if() logic into the return.

Also, allowsMisalignedMemoryAccesses won't have been called if Align >= VT.getSizeInBytes().


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63246