[MIRParser] Diagnose too large align values in MachineMemOperands

[foad]

Summary

When parsing MachineMemOperands, MIRParser treated the "align" keyword
the same as "basealign". Really "basealign" should specify the
alignment of the MachinePointerInfo base value, and "align" should
specify the alignment of that base value plus the offset.

This worked OK when the specified alignment was no larger than the
alignment of the offset, but in cases like this it just caused
confusion:

STW killed %18, 4, %stack.1.ap2.i.i :: (store (s32) into %stack.1.ap2.i.i + 4, align 8)

MIRPrinter would never have printed this, with an offset of 4 but an
align of 8, so it must have been written by hand. MIRParser would
interpret "align 8" as "basealign 8", but I think it is better to give
an error and force the user to write "basealign 8" if that is what they
really meant.

[arsenm]

Needs parser test

[rampitec]

llvm/test/CodeGen/AMDGPU/merge-global-load-store.mir

413

Well, this is unlikely to happen in real life, but this was exactly my point writing this torture test: basealign is 4, and we have managed to know it is misaligned by 8, so ptr +4 has exactly align 8.

I believe MIR parser is correct, it just does what is written.

[foad]

llvm/test/CodeGen/AMDGPU/merge-global-load-store.mir

413

it just does what is written

No, it takes the "align" value as written and uses it to set the basealign field instead.

[rampitec]

LGTM modulo test asked by Matt.

llvm/test/CodeGen/AMDGPU/merge-global-load-store.mir

413

Hm... Ok, given the MachineMemOperand::getAlign() implementation.

[foad]

Add MIR tests.

llvm/lib/CodeGen/MIRParser/MILexer.cpp

return StringSwitch<MIToken::TokenKind>(Identifier)
       .Case("fp128", MIToken::kw_fp128)
       .Case("ppc_fp128", MIToken::kw_ppc_fp128)
       .Case("target-flags", MIToken::kw_target_flags)
       .Case("volatile", MIToken::kw_volatile)
       .Case("non-temporal", MIToken::kw_non_temporal)
       .Case("dereferenceable", MIToken::kw_dereferenceable)
       .Case("invariant", MIToken::kw_invariant)
       .Case("align", MIToken::kw_align)
-      .Case("basealign", MIToken::kw_align)
+      .Case("basealign", MIToken::kw_basealign)
       .Case("addrspace", MIToken::kw_addrspace)
       .Case("stack", MIToken::kw_stack)
       .Case("got", MIToken::kw_got)
       .Case("jump-table", MIToken::kw_jump_table)
       .Case("constant-pool", MIToken::kw_constant_pool)
       .Case("call-entry", MIToken::kw_call_entry)
       .Case("custom", MIToken::kw_custom)
       .Case("liveout", MIToken::kw_liveout)

llvm/lib/CodeGen/MIRParser/MIParser.cpp

if (parseMachinePointerInfo(Ptr))
       return true;
   }
   uint64_t BaseAlignment =
       (Size != MemoryLocation::UnknownSize ? PowerOf2Ceil(Size) : 1);
   AAMDNodes AAInfo;
   MDNode *Range = nullptr;
   while (consumeIfPresent(MIToken::comma)) {
     switch (Token.kind()) {
-    case MIToken::kw_align:
+    case MIToken::kw_align: {
       // align is printed if it is different than size.
-      if (parseAlignment(BaseAlignment))
+      uint64_t Alignment;
+      if (parseAlignment(Alignment))
         return true;
+      if (Ptr.Offset & (Alignment - 1)) {
+        // MachineMemOperand::getAlign never returns a value greater than the
+        // alignment of offset, so this just guards against hand-written MIR
+        // that specifies a large "align" value when it should probably use
+        // "basealign" instead.
+        return error("specified alignment is more aligned than offset");
+      }
+      BaseAlignment = Alignment;
       break;
+    }
     case MIToken::kw_basealign:
       // basealign is printed if it is different than align.
       if (parseAlignment(BaseAlignment))
         return true;
       break;
     case MIToken::kw_addrspace:
       if (parseAddrspace(Ptr.AddrSpace))
         return true;

llvm/test/CodeGen/AMDGPU/merge-global-load-store.mir

bb.0.entry:
 
     ; GCN-LABEL: name: merge_global_load_dword_2_out_of_order
     ; GCN: [[DEF:%[0-9]+]]:vreg_64_align2 = IMPLICIT_DEF
     ; GCN-NEXT: [[GLOBAL_LOAD_DWORDX2_:%[0-9]+]]:vreg_64_align2 = GLOBAL_LOAD_DWORDX2 [[DEF]], 0, 0, implicit $exec :: (load (s64) from `i32 addrspace(1)* undef`, addrspace 1)
     ; GCN-NEXT: [[COPY:%[0-9]+]]:vgpr_32 = COPY [[GLOBAL_LOAD_DWORDX2_]].sub1
     ; GCN-NEXT: [[COPY1:%[0-9]+]]:vgpr_32 = COPY killed [[GLOBAL_LOAD_DWORDX2_]].sub0
     ; GCN-NEXT: S_NOP 0, implicit [[COPY]], implicit [[COPY1]]
     %0:vreg_64_align2 = IMPLICIT_DEF
-    %1:vgpr_32 = GLOBAL_LOAD_DWORD %0, 4, 0, implicit $exec :: (load (s32) from `i32 addrspace(1)* undef` + 4, align 8, addrspace 1)
+    %1:vgpr_32 = GLOBAL_LOAD_DWORD %0, 4, 0, implicit $exec :: (load (s32) from `i32 addrspace(1)* undef` + 4, basealign 8, addrspace 1)
     %2:vgpr_32 = GLOBAL_LOAD_DWORD %0, 0, 0, implicit $exec :: (load (s32) from `float addrspace(1)* undef`, align 4, addrspace 1)
     S_NOP 0, implicit %1, implicit %2
 ...
 
 ---
 name:            merge_global_load_dword_3_out_of_order
 body:             |
   bb.0.entry:

llvm/test/CodeGen/MIR/Generic/aligned-memoperands-err.mir

+# RUN: not llc -run-pass none -o /dev/null %s 2>&1 | FileCheck %s
+
+---
+name: aligned_memoperands
+body: |
+  bb.0:
+    %0:_(p0) = IMPLICIT_DEF
+    ; CHECK: :[[@LINE+1]]:73: specified alignment is more aligned than offset
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef` + 12, align 8)
+...

llvm/test/CodeGen/MIR/Generic/aligned-memoperands.mir

+# RUN: llc -run-pass none -o - %s | FileCheck %s
+
+---
+name: aligned_memoperands
+body: |
+  bb.0:
+    ; CHECK-LABEL: name: aligned_memoperands
+    ; CHECK: [[DEF:%[0-9]+]]:_(p0) = IMPLICIT_DEF
+    ; CHECK-NEXT: [[LOAD:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef`)
+    ; CHECK-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef`, align 2)
+    ; CHECK-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef`)
+    ; CHECK-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef`, align 8)
+    ; CHECK-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef` + 12, align 2)
+    ; CHECK-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef` + 12)
+    ; CHECK-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef` + 12, align 2)
+    ; CHECK-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef` + 12)
+    ; CHECK-NEXT: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[DEF]](p0) :: (load (s32) from `i32* undef` + 12, basealign 8)
+    %0:_(p0) = IMPLICIT_DEF
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef`)
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef`, align 2)
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef`, align 4) ; redundant
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef`, align 8)
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef` + 12, align 2)
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef` + 12, align 4) ; redundant
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef` + 12, basealign 2) ; printed as "align"
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef` + 12, basealign 4) ; redundant
+    %1:_(s32) = G_LOAD %0 :: (load (s32) from `i32* undef` + 12, basealign 8)
+...

llvm/test/CodeGen/Mips/msa/emergency-spill.mir

bb.0.entry:
     $at = LB killed $at_64, 1 :: (load (s8) from %ir.arrayidx3)
     $w1 = FILL_B killed $at
     $w0 = ADDV_B killed $w0, killed $w1
     $w1 = LD_B %stack.4.b.addr, 0 :: (dereferenceable load (s128) from %ir.b.addr)
     $w0 = ADDV_B killed $w1, killed $w0
     ST_B killed $w0, %stack.4.b.addr, 0 :: (store (s128) into %ir.b.addr)
     $w0 = LD_B %stack.4.b.addr, 0 :: (dereferenceable load (s128) from %ir.b.addr)
     ST_B killed $w0, %stack.0.retval, 0 :: (store (s128) into %ir.retval)
-    $v0_64 = LD %stack.0.retval, 0 :: (dereferenceable load (s64) from %ir.20, align 16)
-    $v1_64 = LD %stack.0.retval, 8 :: (dereferenceable load (s64) from %ir.20 + 8, align 16)
+    $v0_64 = LD %stack.0.retval, 0 :: (dereferenceable load (s64) from %ir.20, basealign 16)
+    $v1_64 = LD %stack.0.retval, 8 :: (dereferenceable load (s64) from %ir.20 + 8, basealign 16)
     RetRA implicit $v0_64, implicit $v1_64
 
 ...

llvm/test/CodeGen/PowerPC/stack-coloring-vararg.mir

bb.0.entry:
     STFD %15, 88, %stack.2 :: (store (s64))
     %16:crbitrc = IMPLICIT_DEF
     BC killed %16, %bb.2
     B %bb.1
 
   bb.1.if.then6.i.i:
     LIFETIME_START %stack.1.ap2.i.i
     %17:gprc = LWZ 8, $zero :: (load (s32), align 8)
-    STW killed %17, 8, %stack.1.ap2.i.i :: (store (s32) into %stack.1.ap2.i.i + 8, align 8)
+    STW killed %17, 8, %stack.1.ap2.i.i :: (store (s32) into %stack.1.ap2.i.i + 8, basealign 8)
     %18:gprc = LWZ 4, $zero :: (load (s32))
-    STW killed %18, 4, %stack.1.ap2.i.i :: (store (s32) into %stack.1.ap2.i.i + 4, align 8)
+    STW killed %18, 4, %stack.1.ap2.i.i :: (store (s32) into %stack.1.ap2.i.i + 4, basealign 8)
     %19:gprc = LWZ 0, $zero :: (load (s32), align 8)
-    STW killed %19, 0, %stack.1.ap2.i.i :: (store (s32) into %stack.1.ap2.i.i, align 8)
+    STW killed %19, 0, %stack.1.ap2.i.i :: (store (s32) into %stack.1.ap2.i.i, basealign 8)
     BLR implicit $lr, implicit $rm
 
   bb.2.format_reason_ap.exit.i:
     LIFETIME_START %stack.0.buf.i.i
     ADJCALLSTACKDOWN 8, 0, implicit-def dead $r1, implicit $r1
     %20:gprc = ADDI %stack.0.buf.i.i, 0
     $r3 = COPY %20
     BL @fprintf, csr_svr432, implicit-def dead $lr, implicit $rm, implicit $r3, implicit-def $r1
     ADJCALLSTACKUP 8, 0, implicit-def dead $r1, implicit $r1
     BLR implicit $lr, implicit $rm
 
 ...

llvm/test/DebugInfo/AArch64/asan-stack-vars.mir

bb.0.entry:
     $x13 = LDRXui $x13, target-flags(aarch64-pageoff, aarch64-got, aarch64-nc) @__stack_chk_guard
     $x13 = LDRXui killed $x13, 0 :: (dereferenceable invariant load (s64) from @__stack_chk_guard)
     STRXui killed renamable $x13, $sp, 39 :: (volatile store (s64) into %stack.0.StackGuardSlot)
     renamable $x11 = LDRXui killed renamable $x11, 0 :: (load (s64) from @__asan_shadow_memory_dynamic_address)
     renamable $x13 = ADDXri renamable $x10, 32, 0, debug-location !35
     renamable $x14 = ADDXri renamable $x10, 64, 0, debug-location !35
     $x15 = MOVZXi 35507, 0, debug-location !35
     $x15 = MOVKXi $x15, 16821, 16, debug-location !35
-    STRXui killed renamable $x15, $sp, 24, debug-location !35 :: (store (s768) into %stack.3.MyAlloca, align 32)
-    STRXui killed renamable $x9, $sp, 25, debug-location !35 :: (store (s768) into %stack.3.MyAlloca + 1, align 32)
-    STRXui killed renamable $x8, $sp, 26, debug-location !35 :: (store (s768) into %stack.3.MyAlloca + 2, align 32)
+    STRXui killed renamable $x15, $sp, 24, debug-location !35 :: (store (s768) into %stack.3.MyAlloca, basealign 32)
+    STRXui killed renamable $x9, $sp, 25, debug-location !35 :: (store (s768) into %stack.3.MyAlloca + 1, basealign 32)
+    STRXui killed renamable $x8, $sp, 26, debug-location !35 :: (store (s768) into %stack.3.MyAlloca + 2, basealign 32)
     renamable $x8 = UBFMXri renamable $x10, 3, 63, debug-location !35
     renamable $x9 = ADDXrs renamable $x11, renamable $x10, 67, debug-location !35
     $x15 = MOVZXi 61937, 0, debug-location !35
     $x15 = MOVKXi $x15, 61937, 16, debug-location !35
     $x15 = MOVKXi $x15, 62194, 48, debug-location !35
     STRXroX killed renamable $x15, renamable $x8, renamable $x11, 0, 0, debug-location !35 :: (store (s64) into %ir.15, align 1)
     renamable $x15 = ADDXrs renamable $x8, renamable $x11, 0, debug-location !35
     $w16 = MOVZWi 62451, 0, debug-location !35