[TargetLowering] Add hook for adding target MMO flags when doing ISel.

[gberry]

Summary

Also add support for printing and parsing target MMO flags.

[hfinkel]

Given that I understand the use case here to be the desire to translate metadata on loads into something at the MI level:

Did you consider just adding the metadata operands as MI-level metadata operands to the load? I believe that MI-level metadata operands are only currently used for DBG_VALUEs, but I can imagine using them for more-general information passing. I question came up just the other day (again) about preserving the !unpredictable metadata on branches for MI-level transformations, and so I'm wondering whether it is worth exploring a solution that can handle more than memory accesses.

Alternatively, MMOs currently hold a bunch of pointers to various kinds of Metadata. Should we extend this in some way? One option, for example, would be to allow targets to subclass MachinePointerInfo (and, so, we'd always construct and free these via some target callback), thus allowing the target to add whatever it wants.

All that having been said, AArch64 already uses target MMO flags (for suppressLdStPair), and I'm completely in favor of adding some way to get these in and out of MIR. It would be nice to also let the target callbacks provide names. The syntax could just be to use a quoted string, for example. I believe that's unambiguous.

[gberry]

In D34962#798587, @hfinkel wrote:

Given that I understand the use case here to be the desire to translate metadata on loads into something at the MI level:

Did you consider just adding the metadata operands as MI-level metadata operands to the load? I believe that MI-level metadata operands are only currently used for DBG_VALUEs, but I can imagine using them for more-general information passing. I question came up just the other day (again) about preserving the !unpredictable metadata on branches for MI-level transformations, and so I'm wondering whether it is worth exploring a solution that can handle more than memory accesses.

Alternatively, MMOs currently hold a bunch of pointers to various kinds of Metadata. Should we extend this in some way? One option, for example, would be to allow targets to subclass MachinePointerInfo (and, so, we'd always construct and free these via some target callback), thus allowing the target to add whatever it wants.

All that having been said, AArch64 already uses target MMO flags (for suppressLdStPair), and I'm completely in favor of adding some way to get these in and out of MIR. It would be nice to also let the target callbacks provide names. The syntax could just be to use a quoted string, for example. I believe that's unambiguous.

Since I only needed one bit on some loads, the target MMO flags seemed like a perfect fit. I did consider adding target hooks for printing/parsing these flag values, but it didn't seem worth the effort, since I think it will make parsing these particularly complex (e.g. we would need custom token types per target etc.). If we made them quoted strings, that might make it simple enough to be worth while.

[hfinkel]

In D34962#799842, @gberry wrote:

In D34962#798587, @hfinkel wrote:

Given that I understand the use case here to be the desire to translate metadata on loads into something at the MI level:

Did you consider just adding the metadata operands as MI-level metadata operands to the load? I believe that MI-level metadata operands are only currently used for DBG_VALUEs, but I can imagine using them for more-general information passing. I question came up just the other day (again) about preserving the !unpredictable metadata on branches for MI-level transformations, and so I'm wondering whether it is worth exploring a solution that can handle more than memory accesses.

Alternatively, MMOs currently hold a bunch of pointers to various kinds of Metadata. Should we extend this in some way? One option, for example, would be to allow targets to subclass MachinePointerInfo (and, so, we'd always construct and free these via some target callback), thus allowing the target to add whatever it wants.

All that having been said, AArch64 already uses target MMO flags (for suppressLdStPair), and I'm completely in favor of adding some way to get these in and out of MIR. It would be nice to also let the target callbacks provide names. The syntax could just be to use a quoted string, for example. I believe that's unambiguous.

Since I only needed one bit on some loads, the target MMO flags seemed like a perfect fit. I did consider adding target hooks for printing/parsing these flag values, but it didn't seem worth the effort, since I think it will make parsing these particularly complex (e.g. we would need custom token types per target etc.). If we made them quoted strings, that might make it simple enough to be worth while.

I'd prefer that they be quoted strings; otherwise, I'm happy with the direction.

[MatzeB]

There is already precedent with
TargetInstrInfo::getSerializableTargetIndices(), getSerializableDirectMachineOperandTargetFlags() and getSerializableBitmaskMachineOperandTargetFlags()

Maybe you can add something for MMO flags in a similar way?

[gberry]

Update to add target hook to print/parse target MMO flags as string literals in MIR text

[hfinkel]

This LGTM.

The getMMOFlags addition can be a separate patch, right? If so, please separate it.

I'd also really like a comment added to include/llvm/CodeGen/MachineMemOperand.h, around here:

// Reserved for use by target-specific passes.
MOTargetFlag1 = 1u << 6,
MOTargetFlag2 = 1u << 7,
MOTargetFlag3 = 1u << 8,

reminding us that if we add more flags here we also need to update the MIR printing/parsing code.

include/llvm/Target/TargetLowering.h

public:
   /// which allows a CPU to reuse the result of a previous load indefinitely,
   /// even if a cache-coherent store is performed by another CPU.  The default
   /// implementation does nothing.
   virtual SDValue prepareVolatileOrAtomicLoad(SDValue Chain, const SDLoc &DL,
                                               SelectionDAG &DAG) const {
     return Chain;
   }
 
+  /// This callback is used to inspect load/store instructions and add
+  /// target-specific MachineMemOperand flags to them.  The default
+  /// implementation does nothing.
+  virtual MachineMemOperand::Flags getMMOFlags(const Instruction &I) const {
+    return MachineMemOperand::MONone;
+  }
+
   /// This callback is invoked by the type legalizer to legalize nodes with an
   /// illegal operand type but legal result types.  It replaces the
   /// LowerOperation callback in the type Legalizer.  The reason we can not do
   /// away with LowerOperation entirely is that LegalizeDAG isn't yet ready to
   /// use this callback.
   ///
   /// TODO: Consider merging with ReplaceNodeResults.
   ///

lib/CodeGen/MIRParser/MILexer.h

enum TokenKind {
     kw_double,
     kw_x86_fp80,
     kw_fp128,
     kw_ppc_fp128,
     kw_target_flags,
     kw_volatile,
     kw_non_temporal,
     kw_invariant,
+    kw_flag1,
+    kw_flag2,
+    kw_flag3,
     kw_align,
     kw_stack,
     kw_got,
     kw_jump_table,
     kw_constant_pool,
     kw_call_entry,
     kw_liveout,
     kw_address_taken,
bool isRegisterFlag() const {
     return Kind == kw_implicit || Kind == kw_implicit_define ||
            Kind == kw_def || Kind == kw_dead || Kind == kw_killed ||
            Kind == kw_undef || Kind == kw_internal ||
            Kind == kw_early_clobber || Kind == kw_debug_use;
   }
 
   bool isMemoryOperandFlag() const {
     return Kind == kw_volatile || Kind == kw_non_temporal ||
-           Kind == kw_dereferenceable || Kind == kw_invariant;
+           Kind == kw_dereferenceable || Kind == kw_invariant ||
+           Kind == kw_flag1 || Kind == kw_flag2 || Kind == kw_flag3;
   }
 
   bool is(TokenKind K) const { return Kind == K; }
 
   bool isNot(TokenKind K) const { return Kind != K; }
 
   StringRef::iterator location() const { return Range.begin(); }
 

lib/CodeGen/MIRParser/MILexer.cpp

return StringSwitch<MIToken::TokenKind>(Identifier)
       .Case("x86_fp80", MIToken::kw_x86_fp80)
       .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("flag1", MIToken::kw_flag1)
+      .Case("flag2", MIToken::kw_flag2)
+      .Case("flag3", MIToken::kw_flag3)
       .Case("align", MIToken::kw_align)
       .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("liveout", MIToken::kw_liveout)
       .Case("address-taken", MIToken::kw_address_taken)

lib/CodeGen/MIRParser/MIParser.cpp

case MIToken::kw_non_temporal:
     Flags |= MachineMemOperand::MONonTemporal;
     break;
   case MIToken::kw_dereferenceable:
     Flags |= MachineMemOperand::MODereferenceable;
     break;
   case MIToken::kw_invariant:
     Flags |= MachineMemOperand::MOInvariant;
     break;
-  // TODO: parse the target specific memory operand flags.
+  case MIToken::kw_flag1:
+    Flags |= MachineMemOperand::MOTargetFlag1;
+    break;
+  case MIToken::kw_flag2:
+    Flags |= MachineMemOperand::MOTargetFlag2;
+    break;
+  case MIToken::kw_flag3:
+    Flags |= MachineMemOperand::MOTargetFlag3;
+    break;
   default:
     llvm_unreachable("The current token should be a memory operand flag");
   }
   if (OldFlags == Flags)
     // We know that the same flag is specified more than once when the flags
     // weren't modified.
     return error("duplicate '" + Token.stringValue() + "' memory operand flag");
   lex();

lib/CodeGen/MIRPrinter.cpp

OS << (CmpInst::isIntPredicate(Pred) ? "int" : "float") << "pred("
        << CmpInst::getPredicateName(Pred) << ')';
     break;
   }
   }
 }
 
 void MIPrinter::print(const MachineMemOperand &Op) {
   OS << '(';
-  // TODO: Print operand's target specific flags.
   if (Op.isVolatile())
     OS << "volatile ";
   if (Op.isNonTemporal())
     OS << "non-temporal ";
   if (Op.isDereferenceable())
     OS << "dereferenceable ";
   if (Op.isInvariant())
     OS << "invariant ";
+  if (Op.getFlags() & MachineMemOperand::MOTargetFlag1)
+    OS << "flag1 ";
+  if (Op.getFlags() & MachineMemOperand::MOTargetFlag2)
+    OS << "flag2 ";
+  if (Op.getFlags() & MachineMemOperand::MOTargetFlag3)
+    OS << "flag3 ";
+
   if (Op.isLoad())
     OS << "load ";
   else {
     assert(Op.isStore() && "Non load machine operand must be a store");
     OS << "store ";
   }
 
   if (Op.getSynchScope() == SynchronizationScope::SingleThread)

lib/CodeGen/MachineInstr.cpp

void MachineMemOperand::print(raw_ostream &OS, ModuleSlotTracker &MST) const {
   }
 
   if (isNonTemporal())
     OS << "(nontemporal)";
   if (isDereferenceable())
     OS << "(dereferenceable)";
   if (isInvariant())
     OS << "(invariant)";
+  if (getFlags() & MOTargetFlag1)
+    OS << "(flag1)";
+  if (getFlags() & MOTargetFlag2)
+    OS << "(flag2)";
+  if (getFlags() & MOTargetFlag3)
+    OS << "(flag3)";
 }
 
 //===----------------------------------------------------------------------===//
 // MachineInstr Implementation
 //===----------------------------------------------------------------------===//
 
 void MachineInstr::addImplicitDefUseOperands(MachineFunction &MF) {
   if (MCID->ImplicitDefs)

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

for (unsigned i = 0; i != NumValues; ++i, ++ChainI) {
     if (isVolatile)
       MMOFlags |= MachineMemOperand::MOVolatile;
     if (isNonTemporal)
       MMOFlags |= MachineMemOperand::MONonTemporal;
     if (isInvariant)
       MMOFlags |= MachineMemOperand::MOInvariant;
     if (isDereferenceable)
       MMOFlags |= MachineMemOperand::MODereferenceable;
+    MMOFlags |= TLI.getMMOFlags(I);
 
     SDValue L = DAG.getLoad(ValueVTs[i], dl, Root, A,
                             MachinePointerInfo(SV, Offsets[i]), Alignment,
                             MMOFlags, AAInfo, Ranges);
 
     Values[i] = L;
     Chains[ChainI] = L.getValue(1);
   }
void SelectionDAGBuilder::visitStore(const StoreInst &I) {
   AAMDNodes AAInfo;
   I.getAAMetadata(AAInfo);
 
   auto MMOFlags = MachineMemOperand::MONone;
   if (I.isVolatile())
     MMOFlags |= MachineMemOperand::MOVolatile;
   if (I.getMetadata(LLVMContext::MD_nontemporal) != nullptr)
     MMOFlags |= MachineMemOperand::MONonTemporal;
+  MMOFlags |= TLI.getMMOFlags(I);
 
   // An aggregate load cannot wrap around the address space, so offsets to its
   // parts don't wrap either.
   SDNodeFlags Flags;
   Flags.setNoUnsignedWrap(true);
 
   unsigned ChainI = 0;
   for (unsigned i = 0; i != NumValues; ++i, ++ChainI) {

test/CodeGen/MIR/AArch64/target-memoperands.mir

+# RUN: llc -mtriple=aarch64-none-linux-gnu -run-pass none -o - %s | FileCheck %s
+
+--- |
+
+  define void @target_memoperands() {
+    ret void
+  }
+
+...
+---
+# CHECK-LABEL: name: target_memoperands
+# CHECK: %1(s64) = G_LOAD %0(p0) :: (flag1 load 8)
+# CHECK: %2(s32) = G_LOAD %0(p0) :: (flag2 load 4)
+# CHECK: %3(s16) = G_LOAD %0(p0) :: (flag3 load 2)
+# CHECK: G_STORE %3(s16), %0(p0) :: (flag1 store 2)
+# CHECK: G_STORE %2(s32), %0(p0) :: (flag2 store 4)
+# CHECK: G_STORE %1(s64), %0(p0) :: (flag3 store 8)
+name:            target_memoperands
+body: |
+  bb.0:
+
+    %0:_(p0) = COPY %x0
+    %1:_(s64) = G_LOAD %0(p0) :: (flag1 load 8)
+    %2:_(s32) = G_LOAD %0(p0) :: (flag2 load 4)
+    %3:_(s16) = G_LOAD %0(p0) :: (flag3 load 2)
+    G_STORE %3(s16), %0(p0) :: (flag1 store 2)
+    G_STORE %2(s32), %0(p0) :: (flag2 store 4)
+    G_STORE %1(s64), %0(p0) :: (flag3 store 8)
+    RET_ReallyLR
+...