Consistently use MemoryLocation::UnknownSize to indicate unknown access size

[kparzysz]

Summary

1. Change the software pipeliner to use unknown size instead of dropping memory operands. It used to do it before, but MachineInstr::mayAlias did not handle it correctly.
2. Recognize UnknownSize in MachineInstr::mayAlias.
3. Print and parse UnknownSize in MIR.

[kparzysz]

Ping.

[kparzysz]

Ping.

[MatzeB]

First: My experience with MachineMemOperands is limited, so feel free to correct me where necessary.

About 1): I was always told that machine memory operands are optional to give additional information to alias analysis or relax atomicity constraints. However I assumed one of the basic rules is that dropping them will not introduce correctness problems. Operations without machine mem operands should be handled as conservatively as possible.

About 2), 3): I'd be fine with that if we have a good use case. But shouldn't we naturally know the size of memory operations in the backend? I can't think of any architecture right now where we wouldn't be able to know the size of a memory operation...

[kparzysz]

The practical application of this is to indicate aliasing with a given memory object, but without specifying exactly which part of the object is accessed (since it may be unknown). Dropping memory operands altogether will alias the operation with all other objects, so it will be correct, but overly conservative.

[MatzeB]

In D50339#1206109, @kparzysz wrote:

The practical application of this is to indicate aliasing with a given memory object, but without specifying exactly which part of the object is accessed (since it may be unknown). Dropping memory operands altogether will alias the operation with all other objects, so it will be correct, but overly conservative.

Okay, it sounded to me like you are fixing a correctness problem, but if this is about allowing more information in the mem operands I'm fine with it.

lib/CodeGen/MIRParser/MILexer.h

enum TokenKind {
     kw_call_entry,
     kw_liveout,
     kw_address_taken,
     kw_landing_pad,
     kw_liveins,
     kw_successors,
     kw_floatpred,
     kw_intpred,
+    kw_unknown_size,
 
     // Named metadata keywords
     md_tbaa,
     md_alias_scope,
     md_noalias,
     md_range,
     md_diexpr,
 

lib/CodeGen/MIRParser/MILexer.cpp

return StringSwitch<MIToken::TokenKind>(Identifier)
       .Case("call-entry", MIToken::kw_call_entry)
       .Case("liveout", MIToken::kw_liveout)
       .Case("address-taken", MIToken::kw_address_taken)
       .Case("landing-pad", MIToken::kw_landing_pad)
       .Case("liveins", MIToken::kw_liveins)
       .Case("successors", MIToken::kw_successors)
       .Case("floatpred", MIToken::kw_floatpred)
       .Case("intpred", MIToken::kw_intpred)
+      .Case("unknown-size", MIToken::kw_unknown_size)
       .Default(MIToken::Identifier);
 }
 
 static Cursor maybeLexIdentifier(Cursor C, MIToken &Token) {
   if (!isalpha(C.peek()) && C.peek() != '_')
     return None;
   auto Range = C;
   while (isIdentifierChar(C.peek()))

lib/CodeGen/MIRParser/MIParser.cpp

 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/AsmParser/SlotMapping.h"
 #include "llvm/CodeGen/MIRPrinter.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
Order = StringSwitch<AtomicOrdering>(Token.stringValue())
               .Case("seq_cst", AtomicOrdering::SequentiallyConsistent)
               .Default(AtomicOrdering::NotAtomic);
 
   if (Order != AtomicOrdering::NotAtomic) {
     lex();
     return false;
   }
 
-  return error("expected an atomic scope, ordering or a size integer literal");
+  return error("expected an atomic scope, ordering or a size specification");
 }
 
 bool MIParser::parseMachineMemoryOperand(MachineMemOperand *&Dest) {
   if (expectAndConsume(MIToken::lparen))
     return true;
   MachineMemOperand::Flags Flags = MachineMemOperand::MONone;
   while (Token.isMemoryOperandFlag()) {
     if (parseMemoryOperandFlag(Flags))
bool MIParser::parseMachineMemoryOperand(MachineMemOperand *&Dest) {
   // Up to two atomic orderings (cmpxchg provides guarantees on failure).
   AtomicOrdering Order, FailureOrder;
   if (parseOptionalAtomicOrdering(Order))
     return true;
 
   if (parseOptionalAtomicOrdering(FailureOrder))
     return true;
 
-  if (Token.isNot(MIToken::IntegerLiteral))
-    return error("expected the size integer literal after memory operation");
+  if (Token.isNot(MIToken::IntegerLiteral) &&
+      Token.isNot(MIToken::kw_unknown_size))
+    return error("expected the size integer literal or 'unknown-size' after "
+                 "memory operation");
   uint64_t Size;
+  if (Token.is(MIToken::IntegerLiteral)) {
     if (getUint64(Size))
       return true;
+  } else if (Token.is(MIToken::kw_unknown_size)) {
+    Size = MemoryLocation::UnknownSize;
+  }
   lex();
 
   MachinePointerInfo Ptr = MachinePointerInfo();
   if (Token.is(MIToken::Identifier)) {
     const char *Word =
         ((Flags & MachineMemOperand::MOLoad) &&
          (Flags & MachineMemOperand::MOStore))
             ? "on"
             : Flags & MachineMemOperand::MOLoad ? "from" : "into";
     if (Token.stringValue() != Word)
       return error(Twine("expected '") + Word + "'");
     lex();
 
     if (parseMachinePointerInfo(Ptr))
       return true;
   }
-  unsigned BaseAlignment = Size;
+  unsigned BaseAlignment = (Size != MemoryLocation::UnknownSize ? Size : 1);
   AAMDNodes AAInfo;
   MDNode *Range = nullptr;
   while (consumeIfPresent(MIToken::comma)) {
     switch (Token.kind()) {
     case MIToken::kw_align:
       if (parseAlignment(BaseAlignment))
         return true;
       break;

lib/CodeGen/MachineInstr.cpp

bool MachineInstr::mayAlias(AliasAnalysis *AA, MachineInstr &Other,
   //
   // FIXME: Modify API to hide this math from "user"
   // Even before we go to AA we can reason locally about some
   // memory objects. It can save compile time, and possibly catch some
   // corner cases not currently covered.
 
   int64_t OffsetA = MMOa->getOffset();
   int64_t OffsetB = MMOb->getOffset();
-
   int64_t MinOffset = std::min(OffsetA, OffsetB);
-  int64_t WidthA = MMOa->getSize();
-  int64_t WidthB = MMOb->getSize();
+
+  uint64_t WidthA = MMOa->getSize();
+  uint64_t WidthB = MMOb->getSize();
+  bool KnownWidthA = WidthA != MemoryLocation::UnknownSize;
+  bool KnownWidthB = WidthB != MemoryLocation::UnknownSize;
+
   const Value *ValA = MMOa->getValue();
   const Value *ValB = MMOb->getValue();
   bool SameVal = (ValA && ValB && (ValA == ValB));
   if (!SameVal) {
     const PseudoSourceValue *PSVa = MMOa->getPseudoValue();
     const PseudoSourceValue *PSVb = MMOb->getPseudoValue();
     if (PSVa && ValB && !PSVa->mayAlias(&MFI))
       return false;
     if (PSVb && ValA && !PSVb->mayAlias(&MFI))
       return false;
     if (PSVa && PSVb && (PSVa == PSVb))
       SameVal = true;
   }
 
   if (SameVal) {
+    if (!KnownWidthA || !KnownWidthB)
+      return true;
     int64_t MaxOffset = std::max(OffsetA, OffsetB);
     int64_t LowWidth = (MinOffset == OffsetA) ? WidthA : WidthB;
     return (MinOffset + LowWidth > MaxOffset);
   }
 
   if (!AA)
     return true;
 
   if (!ValA || !ValB)
     return true;
 
   assert((OffsetA >= 0) && "Negative MachineMemOperand offset");
   assert((OffsetB >= 0) && "Negative MachineMemOperand offset");
 
-  int64_t Overlapa = WidthA + OffsetA - MinOffset;
-  int64_t Overlapb = WidthB + OffsetB - MinOffset;
+  int64_t OverlapA = KnownWidthA ? WidthA + OffsetA - MinOffset
+                                 : MemoryLocation::UnknownSize;
+  int64_t OverlapB = KnownWidthB ? WidthB + OffsetB - MinOffset
+                                 : MemoryLocation::UnknownSize;
 
   AliasResult AAResult = AA->alias(
-      MemoryLocation(ValA, Overlapa,
+      MemoryLocation(ValA, OverlapA,
                      UseTBAA ? MMOa->getAAInfo() : AAMDNodes()),
-      MemoryLocation(ValB, Overlapb,
+      MemoryLocation(ValB, OverlapB,
                      UseTBAA ? MMOb->getAAInfo() : AAMDNodes()));
 
   return (AAResult != NoAlias);
 }
 
 /// hasOrderedMemoryRef - Return true if this instruction may have an ordered
 /// or volatile memory reference, or if the information describing the memory
 /// reference is not available. Return false if it is known to have no ordered

lib/CodeGen/MachineOperand.cpp

 //===- lib/CodeGen/MachineOperand.cpp -------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file Methods common to all machine operands.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Analysis/Loads.h"
+#include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/CodeGen/MIRPrinter.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineJumpTableInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/IR/Constants.h"
void MachineMemOperand::print(raw_ostream &OS, ModuleSlotTracker &MST,
 
   printSyncScope(OS, Context, getSyncScopeID(), SSNs);
 
   if (getOrdering() != AtomicOrdering::NotAtomic)
     OS << toIRString(getOrdering()) << ' ';
   if (getFailureOrdering() != AtomicOrdering::NotAtomic)
     OS << toIRString(getFailureOrdering()) << ' ';
 
+  if (getSize() == MemoryLocation::UnknownSize)
+    OS << "unknown-size";
+  else
     OS << getSize();
+
   if (const Value *Val = getValue()) {
     OS << ((isLoad() && isStore()) ? " on " : isLoad() ? " from " : " into ");
     printIRValueReference(OS, *Val, MST);
   } else if (const PseudoSourceValue *PVal = getPseudoValue()) {
     OS << ((isLoad() && isStore()) ? " on " : isLoad() ? " from " : " into ");
     assert(PVal && "Expected a pseudo source value");
     switch (PVal->kind()) {
     case PseudoSourceValue::Stack:

lib/CodeGen/MachinePipeliner.cpp

for (MachineMemOperand *MMO : NewMI.memoperands()) {
     if (MMO->isVolatile() || (MMO->isInvariant() && MMO->isDereferenceable()) ||
         (!MMO->getValue())) {
       NewMemRefs[Refs++] = MMO;
       continue;
     }
     unsigned Delta;
     if (Num != UINT_MAX && computeDelta(OldMI, Delta)) {
       int64_t AdjOffset = Delta * Num;
-      NewMemRefs[Refs++] =
-          MF.getMachineMemOperand(MMO, AdjOffset, MMO->getSize());
+      NewMemRefs[Refs++] = MF.getMachineMemOperand(MMO, AdjOffset,
+                                                   MMO->getSize());
     } else {
-      NewMI.dropMemRefs();
-      return;
+      NewMemRefs[Refs++] = MF.getMachineMemOperand(MMO, 0,
+                                                   MemoryLocation::UnknownSize);
     }
   }
   NewMI.setMemRefs(NewMemRefs, NewMemRefs + NumRefs);
 }
 
 /// Clone the instruction for the new pipelined loop and update the
 /// memory operands, if needed.
 MachineInstr *SwingSchedulerDAG::cloneInstr(MachineInstr *OldMI,

test/CodeGen/MIR/X86/expected-size-integer-after-memory-operation.mir

 ---
 name:            test
 tracksRegLiveness: true
 liveins:
   - { reg: '$rdi' }
 body: |
   bb.0.entry:
     liveins: $rdi
-  ; CHECK: [[@LINE+1]]:53: expected an atomic scope, ordering or a size integer literal
+  ; CHECK: [[@LINE+1]]:53: expected an atomic scope, ordering or a size specification
     $eax = MOV32rm killed $rdi, 1, _, 0, _ :: (load from %ir.a)
     RETQ $eax
 ...
 

test/CodeGen/MIR/X86/expected-size-integer-after-memory-operation2.mir

+# RUN: not llc -march=x86-64 -run-pass none -o /dev/null %s 2>&1 | FileCheck %s
+
+--- |
+
+  define i32 @test(i32* %a) {
+  entry:
+    %b = load i32, i32* %a
+    ret i32 %b
+  }
+
+...
+---
+name:            test
+tracksRegLiveness: true
+liveins:
+  - { reg: '$rdi' }
+body: |
+  bb.0.entry:
+    liveins: $rdi
+  ; CHECK: [[@LINE+1]]:53: expected the size integer literal or 'unknown-size' after memory operation
+    $eax = MOV32rm killed $rdi, 1, _, 0, _ :: (load . from %ir.a)
+    RETQ $eax
+...
+

test/CodeGen/MIR/X86/memory-operands.mir

--- |
   define i8* @undef_value() {
   entry:
     %0 = load i8*, i8** undef, align 8
     ret i8* %0
   }
 
   define void @dummy0() { ret void }
   define void @dummy1() { ret void }
+  define void @dummy2() { ret void }
+  define void @dummy3() { ret void }
 ...
 ---
 name:            test
 tracksRegLiveness: true
 liveins:
   - { reg: '$rdi' }
 body: |
   bb.0.entry:

 name: dummy1
 tracksRegLiveness: true
 stack:
   - { id: 0, size: 4, alignment: 4 }
 body: |
   bb.0:
     $rax = MOV64rm $rsp, 1, _, 0, _ :: (load 8 from %stack.0)
     RETQ $rax
-
+...
+---
+# Test parsing of unknown size in machine memory operands without alignment.
+# CHECK-LABEL: name: dummy2
+# CHECK: $rax = MOV64rm $rsp, 1, $noreg, 0, $noreg :: (load unknown-size from %stack.0, align 1)
+name: dummy2
+tracksRegLiveness: true
+stack:
+  - { id: 0, size: 4, alignment: 4 }
+body: |
+  bb.0:
+    $rax = MOV64rm $rsp, 1, _, 0, _ :: (load unknown-size from %stack.0)
+    RETQ $rax
+...
+---
+# Test parsing of unknown size in machine memory operands with alignment.
+# CHECK-LABEL: name: dummy3
+# CHECK: $rax = MOV64rm $rsp, 1, $noreg, 0, $noreg :: (load unknown-size from %stack.0, align 4)
+name: dummy3
+tracksRegLiveness: true
+stack:
+  - { id: 0, size: 4, alignment: 4 }
+body: |
+  bb.0:
+    $rax = MOV64rm $rsp, 1, _, 0, _ :: (load unknown-size from %stack.0, align 4)
+    RETQ $rax
 ...