Preserve MachinePointerInfo offset for unknown base values

[nikic]

Summary

MachineMemOperands store a MachinePointerInfo (with base value and offset) and a base alignment, where the alignment of the actual memory access is computed from the base alignment and the offset. As such, to correctly determine the alignment of a particular load/store it is important to know both the base alignment and the offset.

However, if the base value is not known, then currently withOffset calls are simple no-ops, so that effectively the alignment ends up being equal to the base alignment. Fortunately this doesn't happen often due to a combination of a) unspecified bases being uncommon and b) a lot of code contains unnecessary redundant MinAlign calls which end up masking the issue. The attached X86 test case is one instance there this did not occur. Previously the store to 15(%rsp) was generating a movaps.

This patch fixes the issue by preserving the offset even if there is no base value.

I'm not sure if this is the right fix -- I was generally left rather confused trying to understand how alignments are treated in codegen. I got the impression that there is a lot of code that's computing alignments and passing them where really a base alignment is expected.

[nikic]

Not sure who to assign for review, so I added the SelectionDAG code owner.

test/CodeGen/AArch64/GlobalISel/legalize-extracts.mir

18

The output here looks a bit odd now, usually if there's a base value and offset this will be printed as load X from XXX + Y. Maybe I should make it print load 8 from unknown + 8 here?

test/CodeGen/X86/byval-odd-size.ll

12

This line used to be vmovaps %xmm0, 15(%rsp).

[nikic]

ping

[nikic]

FTR I submitted https://bugs.llvm.org/show_bug.cgi?id=42361 to keep track of this issue.

include/llvm/CodeGen/MachineMemOperand.h

struct MachinePointerInfo {
   }
 
   explicit MachinePointerInfo(const PseudoSourceValue *v, int64_t offset = 0,
                               uint8_t ID = 0)
       : V(v), Offset(offset), StackID(ID) {
     AddrSpace = v ? v->getAddressSpace() : 0;
   }
 
-  explicit MachinePointerInfo(unsigned AddressSpace = 0)
-      : V((const Value *)nullptr), Offset(0), StackID(0),
+  explicit MachinePointerInfo(unsigned AddressSpace = 0, int64_t offset = 0)
+      : V((const Value *)nullptr), Offset(offset), StackID(0),
         AddrSpace(AddressSpace) {}
 
   explicit MachinePointerInfo(
     PointerUnion<const Value *, const PseudoSourceValue *> v,
     int64_t offset = 0,
     uint8_t ID = 0)
     : V(v), Offset(offset), StackID(ID) {
     if (V) {
       if (const auto *ValPtr = V.dyn_cast<const Value*>())
         AddrSpace = ValPtr->getType()->getPointerAddressSpace();
       else
         AddrSpace = V.get<const PseudoSourceValue*>()->getAddressSpace();
     }
   }
 
   MachinePointerInfo getWithOffset(int64_t O) const {
     if (V.isNull())
-      return MachinePointerInfo(AddrSpace);
+      return MachinePointerInfo(AddrSpace, Offset+O);
     if (V.is<const Value*>())
       return MachinePointerInfo(V.get<const Value*>(), Offset+O, StackID);
     return MachinePointerInfo(V.get<const PseudoSourceValue*>(), Offset+O,
                               StackID);
   }
 
   /// Return true if memory region [V, V+Offset+Size) is known to be
   /// dereferenceable.

test/CodeGen/AArch64/GlobalISel/legalize-extracts.mir

bb.0:
     ; Low part of extraction takes entirity of the low register entirely, so
     ; value stored is forwarded directly from first load.
 
     ; CHECK-LABEL: name: test_extracts_1
     ; CHECK: [[COPY:%[0-9]+]]:_(p0) = COPY $x2
     ; CHECK: [[LOAD:%[0-9]+]]:_(s64) = G_LOAD [[COPY]](p0) :: (load 8, align 16)
     ; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
     ; CHECK: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[COPY]], [[C]](s64)
-    ; CHECK: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[GEP]](p0) :: (load 8)
+    ; CHECK: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[GEP]](p0) :: (load 8 + 8)
     ; CHECK: [[COPY1:%[0-9]+]]:_(s64) = COPY [[LOAD]](s64)
     ; CHECK: G_STORE [[COPY1]](s64), [[COPY]](p0) :: (store 8)
     ; CHECK: RET_ReallyLR
     %0:_(s64) = COPY $x0
     %1:_(s32) = COPY $w1
     %2:_(p0) = COPY $x2
     %3:_(s128) = G_LOAD %2(p0) :: (load 16)
     %4:_(s64) = G_EXTRACT %3(s128), 0
     G_STORE %4(s64), %2(p0) :: (store 8)
     RET_ReallyLR
 ...
 
 ---
 name:            test_extracts_2
 body: |
   bb.0:
     liveins: $w0
 
       ; Low extraction wipes takes whole low register. High extraction is real.
     ; CHECK-LABEL: name: test_extracts_2
     ; CHECK: [[COPY:%[0-9]+]]:_(p0) = COPY $x2
     ; CHECK: [[LOAD:%[0-9]+]]:_(s64) = G_LOAD [[COPY]](p0) :: (load 8, align 16)
     ; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
     ; CHECK: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[COPY]], [[C]](s64)
-    ; CHECK: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[GEP]](p0) :: (load 8)
+    ; CHECK: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[GEP]](p0) :: (load 8 + 8)
     ; CHECK: [[COPY1:%[0-9]+]]:_(s64) = COPY [[LOAD]](s64)
     ; CHECK: [[EXTRACT:%[0-9]+]]:_(s32) = G_EXTRACT [[LOAD1]](s64), 0
     ; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY [[EXTRACT]](s32)
     ; CHECK: G_STORE [[COPY1]](s64), [[COPY]](p0) :: (store 8)
     ; CHECK: G_STORE [[COPY2]](s32), [[COPY]](p0) :: (store 4)
     ; CHECK: RET_ReallyLR
     %0:_(s64) = COPY $x0
     %1:_(s32) = COPY $w1

test/CodeGen/AArch64/GlobalISel/legalize-load-store-s128-unaligned.mir

bb.1:
 
     ; CHECK-LABEL: name: loadstore128_align4
     ; CHECK: liveins: $x0, $x1
     ; CHECK: [[COPY:%[0-9]+]]:_(p0) = COPY $x0
     ; CHECK: [[COPY1:%[0-9]+]]:_(p0) = COPY $x1
     ; CHECK: [[LOAD:%[0-9]+]]:_(s64) = G_LOAD [[COPY]](p0) :: (load 8, align 4)
     ; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
     ; CHECK: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[COPY]], [[C]](s64)
-    ; CHECK: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[GEP]](p0) :: (load 8, align 4)
+    ; CHECK: [[LOAD1:%[0-9]+]]:_(s64) = G_LOAD [[GEP]](p0) :: (load 8 + 8, align 4)
     ; CHECK: G_STORE [[LOAD]](s64), [[COPY1]](p0) :: (store 8, align 4)
     ; CHECK: [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
     ; CHECK: [[GEP1:%[0-9]+]]:_(p0) = G_GEP [[COPY1]], [[C1]](s64)
-    ; CHECK: G_STORE [[LOAD1]](s64), [[GEP1]](p0) :: (store 8, align 4)
+    ; CHECK: G_STORE [[LOAD1]](s64), [[GEP1]](p0) :: (store 8 + 8, align 4)
     ; CHECK: RET_ReallyLR
     %0:_(p0) = COPY $x0
     %1:_(p0) = COPY $x1
     %2:_(s128) = G_LOAD %0(p0) :: (load 16, align 4)
     G_STORE %2(s128), %1(p0) :: (store 16, align 4)
     RET_ReallyLR
 
 ...

test/CodeGen/AArch64/GlobalISel/legalize-load-store.mir

bb.0.entry:
     ; CHECK: [[PTRTOINT:%[0-9]+]]:_(s64) = G_PTRTOINT [[LOAD4]](p0)
     ; CHECK: $x0 = COPY [[PTRTOINT]](s64)
     ; CHECK: [[LOAD5:%[0-9]+]]:_(<2 x s32>) = G_LOAD [[COPY]](p0) :: (load 8)
     ; CHECK: [[BITCAST:%[0-9]+]]:_(s64) = G_BITCAST [[LOAD5]](<2 x s32>)
     ; CHECK: $x0 = COPY [[BITCAST]](s64)
     ; CHECK: [[LOAD6:%[0-9]+]]:_(s64) = G_LOAD [[COPY]](p0) :: (load 8, align 16)
     ; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
     ; CHECK: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[COPY]], [[C]](s64)
-    ; CHECK: [[LOAD7:%[0-9]+]]:_(s64) = G_LOAD [[GEP]](p0) :: (load 8)
+    ; CHECK: [[LOAD7:%[0-9]+]]:_(s64) = G_LOAD [[GEP]](p0) :: (load 8 + 8)
     ; CHECK: [[MV:%[0-9]+]]:_(s128) = G_MERGE_VALUES [[LOAD6]](s64), [[LOAD7]](s64)
     ; CHECK: [[TRUNC:%[0-9]+]]:_(s64) = G_TRUNC [[MV]](s128)
     ; CHECK: $x0 = COPY [[TRUNC]](s64)
     %0:_(p0) = COPY $x0
     %1:_(s1) = G_LOAD %0(p0) :: (load 1)
     %2:_(s32) = G_ANYEXT %1(s1)
     $w0 = COPY %2(s32)
     %3:_(s8) = G_LOAD %0(p0) :: (load 1)
bb.0.entry:
     ; CHECK: G_STORE [[COPY1]](s32), [[COPY]](p0) :: (store 4)
     ; CHECK: [[PTRTOINT:%[0-9]+]]:_(s64) = G_PTRTOINT [[COPY]](p0)
     ; CHECK: G_STORE [[PTRTOINT]](s64), [[COPY]](p0) :: (store 8)
     ; CHECK: G_STORE [[COPY]](p0), [[COPY]](p0) :: (store 8)
     ; CHECK: [[PTRTOINT1:%[0-9]+]]:_(s64) = G_PTRTOINT [[COPY]](p0)
     ; CHECK: G_STORE [[PTRTOINT1]](s64), [[COPY]](p0) :: (store 8, align 16)
     ; CHECK: [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 8
     ; CHECK: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[COPY]], [[C1]](s64)
-    ; CHECK: G_STORE [[PTRTOINT1]](s64), [[GEP]](p0) :: (store 8)
+    ; CHECK: G_STORE [[PTRTOINT1]](s64), [[GEP]](p0) :: (store 8 + 8)
     %0:_(p0) = COPY $x0
     %1:_(s32) = COPY $w1
     %2:_(s1) = G_TRUNC %1(s32)
     G_STORE %2(s1), %0(p0) :: (store 1)
     %3:_(s8) = G_TRUNC %1(s32)
     G_STORE %3(s8), %0(p0) :: (store 1)
     %4:_(s16) = G_TRUNC %1(s32)
     G_STORE %4(s16), %0(p0) :: (store 2)
     G_STORE %1(s32), %0(p0) :: (store 4)
     %5:_(s64) = G_PTRTOINT %0(p0)
     G_STORE %5(s64), %0(p0) :: (store 8)
     G_STORE %0(p0), %0(p0) :: (store 8)
     %6:_(s64) = G_PTRTOINT %0(p0)
     %7:_(s128) = G_MERGE_VALUES %6(s64), %6
     G_STORE %7(s128), %0(p0) :: (store 16)
 ...

test/CodeGen/X86/GlobalISel/legalize-memop-scalar.mir

bb.1 (%ir-block.0):
     ; X64: [[DEF:%[0-9]+]]:_(p0) = IMPLICIT_DEF
     ; X64: [[LOAD:%[0-9]+]]:_(s64) = G_LOAD [[DEF]](p0) :: (load 8)
     ; X64: G_STORE [[LOAD]](s64), [[DEF]](p0) :: (store 8)
     ; X32-LABEL: name: test_memop_s64
     ; X32: [[DEF:%[0-9]+]]:_(p0) = IMPLICIT_DEF
     ; X32: [[LOAD:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load 4, align 8)
     ; X32: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 4
     ; X32: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[DEF]], [[C]](s32)
-    ; X32: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[GEP]](p0) :: (load 4)
+    ; X32: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[GEP]](p0) :: (load 4 + 4)
     ; X32: G_STORE [[LOAD]](s32), [[DEF]](p0) :: (store 4, align 8)
     ; X32: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 4
     ; X32: [[GEP1:%[0-9]+]]:_(p0) = G_GEP [[DEF]], [[C1]](s32)
-    ; X32: G_STORE [[LOAD1]](s32), [[GEP1]](p0) :: (store 4)
+    ; X32: G_STORE [[LOAD1]](s32), [[GEP1]](p0) :: (store 4 + 4)
     %0(p0) = IMPLICIT_DEF
     %1(s64) = G_LOAD %0(p0) :: (load 8)
 
     G_STORE %1, %0 :: (store 8)
 
 ...

test/CodeGen/X86/GlobalISel/legalize-undef.mir

bb.0.entry:
     ; X32: G_STORE [[DEF2]](s16), [[DEF]](p0) :: (store 2)
     ; X32: [[DEF3:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
     ; X32: G_STORE [[DEF3]](s32), [[DEF]](p0) :: (store 4)
     ; X32: [[DEF4:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
     ; X32: [[DEF5:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
     ; X32: G_STORE [[DEF4]](s32), [[DEF]](p0) :: (store 4, align 8)
     ; X32: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 4
     ; X32: [[GEP:%[0-9]+]]:_(p0) = G_GEP [[DEF]], [[C1]](s32)
-    ; X32: G_STORE [[DEF5]](s32), [[GEP]](p0) :: (store 4)
+    ; X32: G_STORE [[DEF5]](s32), [[GEP]](p0) :: (store 4 + 4)
     %5:_(p0) = G_IMPLICIT_DEF
     %0:_(s1) = G_IMPLICIT_DEF
     G_STORE %0, %5 ::(store 1)
     %1:_(s8) = G_IMPLICIT_DEF
     G_STORE %1, %5 ::(store 1)
     %2:_(s16) = G_IMPLICIT_DEF
     G_STORE %2, %5 ::(store 2)
     %3:_(s32) = G_IMPLICIT_DEF
     G_STORE %3, %5 ::(store 4)
     %4:_(s64) = G_IMPLICIT_DEF
     G_STORE %4, %5 ::(store 8)
 ...

test/CodeGen/X86/byval-odd-size.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-linux -mcpu=haswell | FileCheck %s
+%struct = type { [31 x i8] }
+
+declare void @foo(%struct* align 16 byval) nounwind
+
+define void @test1(%struct* nocapture %x) nounwind {
+; CHECK-LABEL: test1:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    subq $40, %rsp
+; CHECK-NEXT:    vmovups 15(%rdi), %xmm0
+; CHECK-NEXT:    vmovups %xmm0, {{[0-9]+}}(%rsp)
+; CHECK-NEXT:    vmovaps (%rdi), %xmm0
+; CHECK-NEXT:    vmovaps %xmm0, (%rsp)
+; CHECK-NEXT:    callq foo
+; CHECK-NEXT:    addq $40, %rsp
+; CHECK-NEXT:    retq
+  call void @foo(%struct* align 16 byval %x)
+  ret void
+}