[RFC] CodeGen: Print/parse LLTs in MachineMemOperands

[arsenm]

Summary

This will currently accept the old number of bytes syntax, and convert
it to a scalar. This should be removed in the near future (I think I
converted all of the tests already, but likely missed a few).

Not sure what the exact syntax and policy should be. We can continue
printing the number of bytes for non-generic instructions to avoid
test churn and only allow non-scalar types for generic instructions.

This will currently print the LLT in parentheses, but accept parsing
the existing integers and implicitly converting to scalar. The
parentheses are a bit ugly, but the parser logic seems unable to deal
without either parentheses or some keyword to indicate the start of a
type.

[thegameg]

In general I think the syntax looks fine. Not sure I get how hard it is to get rid of the parenthesis, but if we end up changing many tests, we should probably do it only once.

Suggestion: maybe add a flag to facilitate the test transition for downstream users? (for CHECK lines, assuming there would be some tests that check for things like load 4 instead of load (s32))

[arsenm]

I completed the test updates, however the diff is at least 3x larger than the phabricator upload maximum size.

Should be able to drop the legacy parsing of the integer size soon in a follow on change

[arsenm]

In D102870#2791326, @thegameg wrote:

In general I think the syntax looks fine. Not sure I get how hard it is to get rid of the parenthesis, but if we end up changing many tests, we should probably do it only once.

Suggestion: maybe add a flag to facilitate the test transition for downstream users? (for CHECK lines, assuming there would be some tests that check for things like load 4 instead of load (s32))

I think this will just contribute to the neverending accumulation of compatibility junk to avoid test updates. At least for globalisel tests, more manual intervention is needed to fixup cases where the scalar is inappropriate. We'll need some memory type verifiers for G_* instructions

[arsenm]

Fix non power of 2 default alignment for weird types

[arsenm]

ping

[aemerson]

Can't we do a token lookahead or something to determine if we need to parse a uint or an LLT?

[arsenm]

In D102870#2848751, @aemerson wrote:

Can't we do a token lookahead or something to determine if we need to parse a uint or an LLT?

This is ready to drop parsing the uint path entirely (although I'll do that as a follow on change)

[arsenm]

fae05692a36f9ebbd201d93c2a6b0f927564d7e6

llvm/lib/CodeGen/MIRParser/MIParser.cpp

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;
 
+  LLT MemoryType;
   if (Token.isNot(MIToken::IntegerLiteral) &&
-      Token.isNot(MIToken::kw_unknown_size))
-    return error("expected the size integer literal or 'unknown-size' after "
+      Token.isNot(MIToken::kw_unknown_size) &&
+      Token.isNot(MIToken::lparen))
+    return error("expected memory LLT, the size integer literal or 'unknown-size' after "
                  "memory operation");
-  uint64_t Size;
+
+  uint64_t Size = MemoryLocation::UnknownSize;
   if (Token.is(MIToken::IntegerLiteral)) {
     if (getUint64(Size))
       return true;
+
+    // Convert from bytes to bits for storage.
+    MemoryType = LLT::scalar(8 * Size);
+    lex();
   } else if (Token.is(MIToken::kw_unknown_size)) {
     Size = MemoryLocation::UnknownSize;
-  }
     lex();
+  } else {
+    if (expectAndConsume(MIToken::lparen))
+      return true;
+    if (parseLowLevelType(Token.location(), MemoryType))
+      return true;
+    if (expectAndConsume(MIToken::rparen))
+      return true;
+
+    Size =  MemoryType.getSizeInBytes();
+  }
 
   MachinePointerInfo Ptr = MachinePointerInfo();
   if (Token.is(MIToken::Identifier)) {
     const char *Word =
         ((Flags & MachineMemOperand::MOLoad) &&
          (Flags & MachineMemOperand::MOStore))
             ? "on"
             : Flags & MachineMemOperand::MOLoad ? "from" : "into";
while (consumeIfPresent(MIToken::comma)) {
     // TODO: Report an error on duplicate metadata nodes.
     default:
       return error("expected 'align' or '!tbaa' or '!alias.scope' or "
                    "'!noalias' or '!range'");
     }
   }
   if (expectAndConsume(MIToken::rparen))
     return true;
-  Dest = MF.getMachineMemOperand(Ptr, Flags, Size, Align(BaseAlignment), AAInfo,
-                                 Range, SSID, Order, FailureOrder);
+  Dest = MF.getMachineMemOperand(Ptr, Flags, MemoryType, Align(BaseAlignment),
+                                 AAInfo, Range, SSID, Order, FailureOrder);
   return false;
 }
 
 bool MIParser::parsePreOrPostInstrSymbol(MCSymbol *&Symbol) {
   assert((Token.is(MIToken::kw_pre_instr_symbol) ||
           Token.is(MIToken::kw_post_instr_symbol)) &&
          "Invalid token for a pre- post-instruction symbol!");
   lex();

llvm/lib/CodeGen/MachineOperand.cpp

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";
+  if (getMemoryType().isValid())
+    OS << '(' << getMemoryType() << ')';
   else
-    OS << getSize();
+    OS << "unknown-size";
 
   if (const Value *Val = getValue()) {
     OS << ((isLoad() && isStore()) ? " on " : isLoad() ? " from " : " into ");
     MIRFormatter::printIRValue(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()) {