[PATCH] D22074: GlobalISel: legalization policy class

[Tim Northover via llvm-commits]

t.p.northover marked 3 inline comments as done.
t.p.northover added a comment.

Updated diff coming soon (I wish you could both answer and upload a patch at the same time).


================
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;
+
----------------
qcolombet wrote:
> 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?
The intent is that legalizeInstr will always succeed. False indicates the instruction was already legal (and so no changes were made). I'll clarify the wording.

================
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
----------------
qcolombet wrote:
> ancillary :).
You know, I thought so too! But Matt quite tactfully pointed out that I'd been mistaken all my life.

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineLegalizer.h:88
@@ +87,3 @@
+  /// before any query is made or incorrect results may be returned.
+  void computeTables();
+
----------------
qcolombet wrote:
> 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?
I'm not too worried about the errors, it's going to be once-per-backend which is pretty miniscule. I'll put an assert in I think.

================
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;
+
----------------
qcolombet wrote:
> Those are the queries you were talking about in computeTables, right?
Yep.

================
Comment at: include/llvm/CodeGen/LowLevelType.h:89
@@ -87,1 +88,3 @@
+  bool isScalar() const { return Kind == Scalar; }
+
   bool isVector() const { return Kind == Vector; }
----------------
qcolombet wrote:
> This diff shouldn’t be in that patch, right?
Probably the previous one (though It think this might be the first use of that function?). I'll move it.


Repository:
  rL LLVM

https://reviews.llvm.org/D22074