[PATCH] D83088: Introduce CfgTraits abstraction

Mon Jul 6 13:05:53 PDT 2020

nhaehnle added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:133
+      ++m_def;
+      if (m_def == m_instr->defs().end()) {
+        ++m_instr;
----------------
arsenm wrote:
> != return early?
The logic is actually subtly broken in the presence of instructions without defs, I just didn't notice it because it currently affects only debug printing logic. Going to fix it.

================
Comment at: llvm/include/llvm/CodeGen/MachineCfgTraits.h:136-138
+          // Prefer to avoid support for bundled instructions as long as we
+          // don't really need it.
+          assert(!m_instr->isBundle());
----------------
arsenm wrote:
> I've been thinking about more aggressively using bundles around call sites to handle waterfall looping around divergent calls with SGPR arguments
Hmm, so what's the correct iteration behavior in the presence of bundles? Iterate over all instructions in the bundle (which is that MachineBasicBlock::instr_iterator does) and only iterate over explicit defs? I think that's what makes the most sense, and what I'm going with for now...

================
Comment at: llvm/lib/CodeGen/MachineCfgTraits.cpp:27-29
+void MachineCfgTraits::Printer::printBlockName(raw_ostream &out,
+                                               MachineBasicBlock *block) const {
+  out << "bb." << block->getNumber();
----------------
arsenm wrote:
> I think this should be added to MachineBasicBlock. The same logic is already repeated in MIRPrinter (and the MBB dump function uses a different prefix)
D83253

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83088