[PATCH] D55988: WIP: RegBankSelect: Support some more complex part mappings

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/RegisterBankInfo.h:629
+  /// be inserted.
+  virtual unsigned getBreakDownCost(const ValueMapping &ValMapping) const {
+    return std::numeric_limits<unsigned>::max();
----------------
I feel we miss a register bank in that prototype (what you'd referenced as CurBank).


================
Comment at: lib/CodeGen/GlobalISel/RegBankSelect.cpp:140
+
+  assert(ValMapping.NumBreakDowns == size(NewVRegs));
+
----------------
Could you add a message in the assert (e.g., we need as many new regs as break downs.


================
Comment at: lib/CodeGen/GlobalISel/RegBankSelect.cpp:206
+    }
+  }
+
----------------
I wonder if we should make this part target specific with a default implementation being what you added.
Indeed, I would expect some target would have fancy instructions to do it in one go ala REG_SEQUENCE.


================
Comment at: lib/CodeGen/GlobalISel/RegBankSelect.cpp:209
+  assert(RepairPt.getNumInsertPoints() == 1 &&
+         "is it possible to have multiple?");
+
----------------
Yes, if we repair a physical register. In practice I've never seen that happen.


================
Comment at: lib/CodeGen/GlobalISel/RegisterBankInfo.cpp:502
+bool RegisterBankInfo::ValueMapping::partsAllUniform() const {
+  assert(NumBreakDowns > 1);
+
----------------
We could return true instead of asserting.
Make sure to document in the doxygen the pre-condition if you want to stick with the assert.


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

https://reviews.llvm.org/D55988