[PATCH] D75098: Add COPY_BR, a terminator form of the COPY instr

[Bill Wendling via Phabricator via llvm-commits]

void marked 7 inline comments as done.
void added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1121
+    return getOpcode() == TargetOpcode::COPY ||
+        getOpcode() == TargetOpcode::COPY_BR;
   }
----------------
nickdesaulniers wrote:
> Does this save a few `getOpcode()` calls? The rest of this CL explicitly compares the opcode against `COPY_BR`? I just worry if this might mess up existing callsites of `isCopy` when `getOpcode() == TargetOpcode::COPY_BR;`.
It's more than just a few. `isCopy()` is used pretty extensively. I want the `COPY_BR` (or `COPY_TERM`) to be exactly like a normal `COPY`, with the only exception that it can come after a terminator.


================
Comment at: llvm/include/llvm/Support/TargetOpcodes.def:101
+/// COPY_BR - This instruction is the terminal version of COPY. The purpose is
+/// to allow copies from terminals to be properly represented (e.g. an
+/// INLINEASM_BR that defines a physical register) without having
----------------
arsenm wrote:
> arsenm wrote:
> > s/terminal/terminator
> COPY_TERM or COPY_TERMINATOR would be better since it isn't really a branch
How about `TCOPY`?


================
Comment at: llvm/include/llvm/Target/Target.td:1126
+  let isIndirectBranch = 1;
+}
 def BUNDLE : StandardPseudoInstruction {
----------------
nickdesaulniers wrote:
> `COPY` also sets `hasNoSchedulingInfo` to `0`. Should `COPY_BR` do that as well?
Possibly, though I had trouble finding where to place its scheduling info. It seems like `COPY` is special and TableGen adds it by default. I couldn't find the place to do that for `COPY_BR`, but I'll take another look.


================
Comment at: llvm/lib/CodeGen/ExpandPostRAPseudos.cpp:191
+  for (auto &MBB : MF) {
+    for (MachineBasicBlock::iterator mi = MBB.begin(), me = MBB.end();
          mi != me;) {
----------------
nickdesaulniers wrote:
> what? if we could use a range based for loop for the `MBB`'s, surely we can for `MI`.
The `++mi` below made me worried, and the fact that MI could be erased.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1397
+  if (InvalidateLiveness)
+    MF.getRegInfo().invalidateLiveness();
   return Changed;
----------------
nickdesaulniers wrote:
> Does it make sense to "sink" register info invalidation into `performSink()`? (pun intended) Since it's already checking the `MI`'s opcode? Or are the two call sites of `performSink()` problematic?  The implementation of `invalidateLiveness()` looks pretty cheap, IMO.
Possibly. If we're going to make `COPY_TERM` (or whatever name we settle on) not specific to `INLINEASM_BR`, then we'll probably need to modify this to accept any case where we sink copies after a terminator. We'll come back to this after other comments settle.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:179
+    const MachineBasicBlock::iterator Term = MBB->getFirstTerminator();
+    if (Term != MBB->end() && Term->getOpcode() == TargetOpcode::INLINEASM_BR)
+      TgtOpc = TargetOpcode::COPY_BR;
----------------
arsenm wrote:
> I would expect this to not special case INLINEASM_BR. This should be any value defined by a terminator instruction
Does this include unconditional jump terminators?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75098/new/

https://reviews.llvm.org/D75098