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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2133-2136
+    if (!!(Flags & MachineMemOperand::MOLoad) &&
+        (Align < 16 || !Subtarget.hasSSE41()))
+      return true;
+    return false;
----------------
RKSimon wrote:
> 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().
Thinking about it more, yeah, the tests seem to agree that it works as intended, even though the `allowsMisalignedMemoryAccesses()` looks backwards.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63246