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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:1121
+    return getOpcode() == TargetOpcode::COPY ||
+        getOpcode() == TargetOpcode::COPY_BR;
   }
----------------
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;`.


================
Comment at: llvm/include/llvm/Target/Target.td:1126
+  let isIndirectBranch = 1;
+}
 def BUNDLE : StandardPseudoInstruction {
----------------
`COPY` also sets `hasNoSchedulingInfo` to `0`. Should `COPY_BR` do that as well?


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


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:1397
+  if (InvalidateLiveness)
+    MF.getRegInfo().invalidateLiveness();
   return Changed;
----------------
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.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:864
+      /* Ignore copies after an INLINEASM_BR. */
+    } else {
+      report("Non-terminator instruction after the first terminator", MI);
----------------
this is hard to read. Is there a nice way to simply this? Maybe negate, and fold into parent `if`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75098