[PATCH] D22074: GlobalISel: legalization policy class

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineLegalizer.h:83
@@ +82,3 @@
+  /// \returns true if the function is modified.
+  bool legalizeInstr(MachineInstr &MI, MachineLegalizeHelper &Helper) const;
+
----------------
I found the returns comment hard to understand :).
I believe that means MI has been legalized successfully?
What happen when we return false? In particular is the function left untouched as nothing as happened or are we in a strange state that we may not be able to recover?

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineLegalizer.h:85
@@ +84,3 @@
+
+  /// Compute any ancillary tables needed to quickly decide how an operation
+  /// should be handled. This must be called after all "set*Action"methods but
----------------
ancillary :).

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineLegalizer.h:88
@@ +87,3 @@
+  /// before any query is made or incorrect results may be returned.
+  void computeTables();
+
----------------
Shouldn’t we hide this function?

For instance, I would have expected that the class call it internally the first time it does a query.
Note: I understand why for compile time reason we would like to expose this API. I am just nervous that if we forget to do it, we end up with a bunch of hard to understand errors.
Alternative, could you add an assert of some sort catch that?

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineLegalizer.h:109
@@ +108,3 @@
+  std::pair<LegalizeAction, LLT> getAction(unsigned Opcode, LLT) const;
+  std::pair<LegalizeAction, LLT> getAction(MachineInstr &MI) const;
+
----------------
Those are the queries you were talking about in computeTables, right?

================
Comment at: include/llvm/CodeGen/LowLevelType.h:89
@@ -87,1 +88,3 @@
+  bool isScalar() const { return Kind == Scalar; }
+
   bool isVector() const { return Kind == Vector; }
----------------
This diff shouldn’t be in that patch, right?


Repository:
  rL LLVM

https://reviews.llvm.org/D22074