[InlineAsm] Steal a bit to denote a register is foldable #70738

nickdesaulniers · 2023-10-30T22:25:56Z

When using the inline asm constraint string "rm" (or "g"), we generally
would like the compiler to choose "r", but it is permitted to choose "m"
if there's register pressure. This is distinct from "r" in which the
register is not permitted to be spilled to the stack.

The decision of which to use must be made at some point. Currently, the
instruction selection frameworks (ISELs) make the choice, and the
register allocators had better be able to handle the result.

Steal a bit from Storage when using registers operands to disambiguate
between the two cases. Add helpers/getters/setters, and print in MIR
when such a register is spillable.

The getter will later be used by the register allocation frameworks (and
asserted by the ISELs) while the setters will be used by the instruction
selection frameworks.

Link: #20571

When using the inline asm constraint string "rm" (or "g"), we generally would like the compiler to choose "r", but it is permitted to choose "m" if there's register pressure. This is distinct from "r" in which the register is not permitted to be spilled to the stack. The decision of which to use must be made at some point. Currently, the instruction selection frameworks (ISELs) make the choice, and the register allocators had better be able to handle the result. Steal a bit from Storage when using registers operands to disambiguate between the two cases. Add helpers/getters/setters, and print in MIR when such a register is spillable. The getter will later be used by the register allocation frameworks (and asserted by the ISELs) while the setters will be used by the instruction selection frameworks.

llvmbot · 2023-10-30T22:26:58Z

@llvm/pr-subscribers-llvm-ir

Author: Nick Desaulniers (nickdesaulniers)

Changes

When using the inline asm constraint string "rm" (or "g"), we generally
would like the compiler to choose "r", but it is permitted to choose "m"
if there's register pressure. This is distinct from "r" in which the
register is not permitted to be spilled to the stack.

The decision of which to use must be made at some point. Currently, the
instruction selection frameworks (ISELs) make the choice, and the
register allocators had better be able to handle the result.

Steal a bit from Storage when using registers operands to disambiguate
between the two cases. Add helpers/getters/setters, and print in MIR
when such a register is spillable.

The getter will later be used by the register allocation frameworks (and
asserted by the ISELs) while the setters will be used by the instruction
selection frameworks.

Full diff: https://github.com/llvm/llvm-project/pull/70738.diff

4 Files Affected:

(modified) llvm/include/llvm/CodeGen/MachineInstr.h (+3)
(modified) llvm/include/llvm/IR/InlineAsm.h (+30-5)
(modified) llvm/lib/CodeGen/MachineInstr.cpp (+23)
(modified) llvm/lib/CodeGen/TargetInstrInfo.cpp (+4)

diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h
index 4877f43e8578d1c..93e8ff389d65673 100644
--- a/llvm/include/llvm/CodeGen/MachineInstr.h
+++ b/llvm/include/llvm/CodeGen/MachineInstr.h
@@ -1364,6 +1364,9 @@ class MachineInstr
     return getOpcode() == TargetOpcode::INLINEASM ||
            getOpcode() == TargetOpcode::INLINEASM_BR;
   }
+  /// Returns true if the memory operand can be folded. Does so by checking the
+  /// InlineAsm::Flag immediate operand at OpId - 1.
+  bool mayFoldInlineAsmMemOp(unsigned OpId) const;
 
   bool isStackAligningInlineAsm() const;
   InlineAsm::AsmDialect getInlineAsmDialect() const;
diff --git a/llvm/include/llvm/IR/InlineAsm.h b/llvm/include/llvm/IR/InlineAsm.h
index 969ad42816a7e52..2d395a53608b0b7 100644
--- a/llvm/include/llvm/IR/InlineAsm.h
+++ b/llvm/include/llvm/IR/InlineAsm.h
@@ -291,18 +291,23 @@ class InlineAsm final : public Value {
   //     Bits 30-16 - A ConstraintCode:: value indicating the original
   //                  constraint code. (MemConstraintCode)
   //   Else:
-  //     Bits 30-16 - The register class ID to use for the operand. (RegClass)
+  //     Bits 29-16 - The register class ID to use for the operand. (RegClass)
+  //     Bit  30    - If the register is permitted to be spilled.
+  //                  (RegMayBeSpilled)
+  //                  Defaults to false "r", may be set for constraints like
+  //                  "rm" (or "g").
   //
-  //   As such, MatchedOperandNo, MemConstraintCode, and RegClass are views of
-  //   the same slice of bits, but are mutually exclusive depending on the
-  //   fields IsMatched then KindField.
+  //   As such, MatchedOperandNo, MemConstraintCode, and
+  //   (RegClass+RegMayBeSpilled) are views of the same slice of bits, but are
+  //   mutually exclusive depending on the fields IsMatched then KindField.
   class Flag {
     uint32_t Storage;
     using KindField = Bitfield::Element<Kind, 0, 3, Kind::Func>;
     using NumOperands = Bitfield::Element<unsigned, 3, 13>;
     using MatchedOperandNo = Bitfield::Element<unsigned, 16, 15>;
     using MemConstraintCode = Bitfield::Element<ConstraintCode, 16, 15, ConstraintCode::Max>;
-    using RegClass = Bitfield::Element<unsigned, 16, 15>;
+    using RegClass = Bitfield::Element<unsigned, 16, 14>;
+    using RegMayBeSpilled = Bitfield::Element<bool, 30, 1>;
     using IsMatched = Bitfield::Element<bool, 31, 1>;
 
 
@@ -413,6 +418,26 @@ class InlineAsm final : public Value {
              "Flag is not a memory or function constraint!");
       Bitfield::set<MemConstraintCode>(Storage, ConstraintCode::Unknown);
     }
+
+    /// Set a bit to denote that while this operand is some kind of register
+    /// (use, def, ...), a memory flag did appear in the original constraint
+    /// list.  This is set by the instruction selection framework, and consumed
+    /// by the register allocator. While the register allocator is generally
+    /// responsible for spilling registers, we need to be able to distinguish
+    /// between registers that the register allocator has permission to spill
+    /// ("rm") vs ones it does not ("r"). This is because the inline asm may use
+    /// instructions which don't support memory addressing modes for that
+    /// operand.
+    void setRegMayBeSpilled(bool B) {
+      assert((isRegDefKind() || isRegDefEarlyClobberKind() || isRegUseKind()) &&
+             "Must be reg");
+      Bitfield::set<RegMayBeSpilled>(Storage, B);
+    }
+    bool getRegMayBeSpilled() const {
+      assert((isRegDefKind() || isRegDefEarlyClobberKind() || isRegUseKind()) &&
+             "Must be reg");
+      return Bitfield::get<RegMayBeSpilled>(Storage);
+    }
   };
 
   static std::vector<StringRef> getExtraInfoNames(unsigned ExtraInfo) {
diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
index 048563cc2bcc4e4..92c789e85a205b4 100644
--- a/llvm/lib/CodeGen/MachineInstr.cpp
+++ b/llvm/lib/CodeGen/MachineInstr.cpp
@@ -1792,6 +1792,12 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST,
       if (F.isUseOperandTiedToDef(TiedTo))
         OS << " tiedto:$" << TiedTo;
 
+      if ((F.isRegDefKind() || F.isRegDefEarlyClobberKind() ||
+           F.isRegUseKind()) &&
+          F.getRegMayBeSpilled()) {
+        OS << " spillable";
+      }
+
       OS << ']';
 
       // Compute the index of the next operand descriptor.
@@ -2526,3 +2532,20 @@ void MachineInstr::insert(mop_iterator InsertBefore,
     tieOperands(Tie1, Tie2);
   }
 }
+
+bool MachineInstr::mayFoldInlineAsmMemOp(unsigned OpId) const {
+  assert(OpId && "expected non-zero operand id");
+  assert(isInlineAsm() && "should only be used on inline asm");
+
+  if (!getOperand(OpId).isReg())
+    return false;
+
+  const MachineOperand &MD = getOperand(OpId - 1);
+  if (!MD.isImm())
+    return false;
+
+  InlineAsm::Flag F(MD.getImm());
+  if (F.isRegUseKind() || F.isRegDefKind() || F.isRegDefEarlyClobberKind())
+    return F.getRegMayBeSpilled();
+  return false;
+}
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index fe7efb73a2dce83..bcf9105ea64ca96 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -1639,6 +1639,10 @@ std::string TargetInstrInfo::createMIROperandComment(
   if (F.isUseOperandTiedToDef(TiedTo))
     OS << " tiedto:$" << TiedTo;
 
+  if ((F.isRegDefKind() || F.isRegDefEarlyClobberKind() || F.isRegUseKind()) &&
+      F.getRegMayBeSpilled())
+    OS << " spillable";
+
   return OS.str();
 }

nickdesaulniers · 2023-10-30T22:27:16Z

Note to reviewers: I'm trying to break up my series into more reviewable pieces. Please let know if you'd prefer to have everything all in one PR. I'm going to try my hand at a stacked PR (though I'm skeptical of that)

MatzeB · 2023-10-30T22:40:46Z

The name "spillable" seems odd to me. I usually only talk about registers being spilled, for an operand I would not use this term...
I would probably describe the situation as a register operand that also supports memory addressing (or can be turned into some other operation that supports memory addressing via TargetInstrInfo::foldMemoryOperand). So I'd rather see some terminology in terms of "memory addressing" or "memory operand" (though "memory operand" may also be misleading as it's often a combination of multiple MachineOperands that together form an address...)

nickdesaulniers · 2023-10-30T22:46:25Z

The name "spillable" seems odd to me.

perhaps "foldable" is better? "memory foldable" ? "may spill", etc?

I usually only talk about registers being spilled

Only MachineOperands that are registers may be marked spillable. They are registers; and only registers.

MatzeB · 2023-10-30T22:51:21Z

They are registers; and only registers.

I would say those operands reference a register. But typically multiple operands use the same registers and the act of spilling affects all operands referencing that (virtual-)register...

nickdesaulniers · 2023-10-30T22:59:29Z

For reference, here's the output from ISEL (from the complete series):

INLINEASM &"# $0 $1" [sideeffect] [attdialect], $0:[reguse:GPR64common spillable], %1:gpr64common

So for an INLINEASM MachineInstr, if we were to look at %1, we have a register MachineOperand. MachineOperand::isReg() would return true.

INLINEASM's first MachineOperand is the string literal of asm to send straight through to the backend.
The second is "extra info" about mayload/maystore/what dialect.
Then we have pairs (well, groups in x86's case) of operands. Generally an immediate that has metadata about the next MachineOperand which is the logical operand of the inline asm.

So "given %1, is it spillable?" is the question this PR seeks to provide an answer for. "rm" is spillable. "r" is a register that is not spillable.

perhaps "foldable" is better? "memory foldable" ? "may spill", etc?

I would say those operands reference a register.

What would you call it then?

qcolombet

In this context, I don't have an issue with calling this spillable as it matches the LiveInterval naming markNotSpillable.
I let you guys sort out the naming, but this LGTM otherwise.

In other words, don't merge before @MatzeB and you agree.

MatzeB · 2023-10-31T16:57:41Z

Calling a LiveInterval "spillable" makes sense to me to, but not a machine operand. Also consider that we can very well spill most of the registers even if this bit is not set on the operands! (We just cannot fold the reload/spills into the instruction)

I'd be fine with AllowsMemoryAddressing or MemoryFoldable; I'd personally not add a Reg prefix to the name but feel free to add it if you think that is important.

nickdesaulniers · 2023-10-31T17:10:00Z

I'd be fine with AllowsMemoryAddressing or MemoryFoldable; I'd personally not add a Reg prefix to the name but feel free to add it if you think that is important.

For the sake of brevity, can we go with just foldable? Or memFoldable?

MatzeB · 2023-10-31T17:17:20Z

For the sake of brevity, can we go with just foldable? Or memFoldable?

Fine with me too.

nickdesaulniers · 2023-10-31T21:43:09Z

For the sake of brevity, can we go with just foldable? Or memFoldable?

Fine with me too.

Done in 14d69cf PTAL

llvm/include/llvm/IR/InlineAsm.h

MatzeB

LGTM

arsenm · 2023-11-01T02:47:07Z

llvm/lib/CodeGen/MachineInstr.cpp

+      if ((F.isRegDefKind() || F.isRegDefEarlyClobberKind() ||
+           F.isRegUseKind()) &&
+          F.getRegMayBeFolded()) {
+        OS << " foldable";


Does this require new MIR printer/parser support and corresponding tests?

Here's a part of MIR I kind of curious about. Maybe you can double check my understanding?

(For the entire complete series I have locally) When I use llc -stop-before or -stop-after to generate the starting point of a new MIR test, foldable is printed as a comment, using my change to TargetInstrInfo::createMIROperandComment below. Example:

INLINEASM &"", 0 /* attdialect */, 1076101130 /* regdef:GR32 spillable */, def %0

But if I use llc -print-after-all (or -print-after=finalize-isel), it's printed via this method.

INLINEASM &"" [attdialect], $0:[regdef:GR32 spillable], %0:gr32

(Note that the MachineOperand is a 32b immediate encoding an InlineAsm::Flag, hence the value 1076101130 which we pretty print as $0:[regdef:GR32 spillable] to be somewhat more understandable).

So for the purposes of testing parsing MIR, I think the answer to the question of "does this need a parser test" is "no" because foldable is only ever printed inside a comment, which is what we'd need to be able to parse for MIR tests. IIUC, it seems the parser consumes the result of TargetInstrInfo::createMIROperandComment and not MachineInstr::print as far as this is concerned (and that result is a comment).

For the purposes of testing printing MIR, nothing will be setting this bit until the instruction selection frameworks later in the series. For those, I will add MIR tests that they use the above MachineInstr::print method to print, but IIUC it seems that you need to use -print-after with a pass that sets this bit in order to observe the result of the above method.

So can the printing test wait until then? Or is there some other way to provide code coverage of MachineInstr::print before then that I'm not thinking of? I could write a unittest if this is really necessary, but there doesn't seem to be much unittests for MIR.

So right, it doesn't require MIR testing. This is one of the areas where the debug output and MIR are not identical but ideally would be. The magic number with comments in inline asm operands are a pain point for MIR test maintenance

nickdesaulniers requested review from qcolombet and MatzeB October 30, 2023 22:26

llvmbot added the llvm:ir label Oct 30, 2023

nickdesaulniers requested review from jyknight, bwendling and efriedma-quic October 30, 2023 22:26

nickdesaulniers requested a review from arsenm October 30, 2023 22:27

nickdesaulniers mentioned this pull request Oct 30, 2023

[TargetInstrInfo] enable foldMemoryOperand for InlineAsm #70743

Merged

This was referenced Oct 30, 2023

inline asm "rm" constraint lowered "m" when "r" would be preferable #20571

Open

[CalcSpillWeights] don't mark live intervals with spillable inlineasm ops as having infinite spill weight #70747

Merged

qcolombet approved these changes Oct 31, 2023

View reviewed changes

nickdesaulniers mentioned this pull request Oct 31, 2023

[X86InstrInfo] support memfold on spillable inline asm #70832

Merged

s/spillable/foldable/

14d69cf

fix typo

b14d86a

nickdesaulniers commented Oct 31, 2023

View reviewed changes

llvm/include/llvm/IR/InlineAsm.h Outdated Show resolved Hide resolved

one more s/spill/fold/

245dfb6

MatzeB approved these changes Oct 31, 2023

View reviewed changes

arsenm reviewed Nov 1, 2023

View reviewed changes

nickdesaulniers changed the title ~~[InlineAsm] Steal a bit to denote a register is spillable~~ [InlineAsm] Steal a bit to denote a register is foldable Nov 1, 2023

nickdesaulniers merged commit 778a484 into llvm:main Nov 3, 2023

nickdesaulniers deleted the asm_rm_core branch November 3, 2023 16:37

[InlineAsm] Steal a bit to denote a register is foldable #70738

[InlineAsm] Steal a bit to denote a register is foldable #70738

Uh oh!

Conversation

nickdesaulniers commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 30, 2023

Uh oh!

nickdesaulniers commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatzeB commented Oct 30, 2023

Uh oh!

nickdesaulniers commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatzeB commented Oct 30, 2023

Uh oh!

nickdesaulniers commented Oct 30, 2023

Uh oh!

qcolombet left a comment

Choose a reason for hiding this comment

Uh oh!

MatzeB commented Oct 31, 2023

Uh oh!

nickdesaulniers commented Oct 31, 2023

Uh oh!

MatzeB commented Oct 31, 2023

Uh oh!

nickdesaulniers commented Oct 31, 2023

Uh oh!

Uh oh!

MatzeB left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickdesaulniers commented Oct 30, 2023 •

edited

Loading

nickdesaulniers commented Oct 30, 2023 •

edited

Loading

nickdesaulniers commented Oct 30, 2023 •

edited

Loading

nickdesaulniers Nov 1, 2023 •

edited

Loading