[PATCH] D21534: GlobalISel: first outline of legalization interface.

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Tim,

Thanks for working on this!

I believe this is work in progress so I did not go into great details to comment on each part of the patch. For instance, for a detailed review, I would have expected the patch to be split into smaller one.

First some general comment:

- I would split the file with the legalization APIs in two:
  - one for the machine pass
  - one for the helper

The rationale is to be able to include them independently.

- I would split the LegalizeHelper in two:
  - One with all the methods you added (narrow, widen, etc), but legalize
  - One with the legalize API and the set action stuff

Put differently, I would have the legalizer info being the actual legalizer. We have one per sub target and it knows how to legalize stuff. It uses the LegalizeHelper for the generic action.

- Replace the Type/EVT by something lower level (Cf. my email)

Basically, we do not need to know what are the types, we only need to know their size and there number of lanes, i.e., I would go with a different types representation for the MI level. It is a much bigger change than just for legalization and we might postpone it, but it is something to keep in mind.

- Add unit test cases for the legalize helper.

I think this is it for the high level comments.

More comments below.

Cheers,
-Quentin


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizeMachineIR.h:39
@@ +38,3 @@
+  /// should be performed before calling this function.
+  bool legalizeInstr(MachineInstr &MI);
+
----------------
I think we need to say something about what happens to the definition and arguments of MI.
My goal was to have transformations that take legal mir and produce legal mir. For that to be possible, we need to preserve the definition and arguments (with what I called the extract and build_sequence operations in the talk.)
We need to say that somewhere. Moreover, we need to say what happen to the extract and build_sequence operations that are created. Do we legalize them now, do we return a list of them, etc.?
I think the latter is preferable but maybe we want to also provide an option for the former.

Also, please comment on what is the boolean supposed to mean.

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:199
@@ +198,3 @@
+  /// \return The newly created instruction.
+  MachineInstr *buildExtract(Type *Ty, unsigned Res, unsigned Op, int Idx);
+
----------------
I can see how this method will be useful to handle vectors.
Just one comment on the extract instruction. In my mind, the extract instruction was more a extract bitfield like instruction and could produce several definitions.
In other words, the instruction supports the following generic syntax:
<def1>, …, <defN> = extract <use>, <startBitIdx1>, <lengthDef1>, …, <startBitIdxN>, <lengthDefN>

The only requirement, again in my mind, was that the extracted bits do not overlap. The reason why I bring that here is because I think it is easier for the optimizers to reason about only one instruction for things that work on the same input (regbankselect in mind), e.g., gpr, gpr = movrrd fpr naturally fit with this semantic of extract instruction and vice versa. Therefore if we try to legalize only “half” of the extract, we may miss that this pattern is available.

To give something more concret with respect to legalization, say we have:
def1 = extract val, ...
def2 = extract val, …

Let say this is illegal and needs to be lowered in loads and stores. We will likely have:
store val
def1 = load @offset1
store val ; <— redundant store
def2 = load @offset2

And things needs to be cleaned up later.
Now, we the other representation:
def1, def2 = extract val, …
we get
store val
def1 = load @offset1 ; <— loads may be combined
def2 = load @offset2

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineLegalizeInfo.h:30
@@ +29,3 @@
+
+/// Holds all the information related to register banks.
+class MachineLegalizeInfo {
----------------
Copy paste :)

================
Comment at: include/llvm/CodeGen/GlobalISel/MachineLegalizeInfo.h:51
@@ +50,3 @@
+  /// Set how a specific G_* operation should be legalized on a given type.
+  void setAction(unsigned Opcode, Type *Ty, LegalizeAction Action) {
+    Actions[std::make_pair(Opcode, Ty)] = Action;
----------------
We’ll actually need more setAction kind of thing.
E.g., right now, there is no way to specify that "fptoint float to i32" is legal whereas "fptoint float to i64" is not, same for truncate, extensions, etc.
What I am saying is that some instructions need both their input and output types to set an action.
Note: This is something broken in SDISel and we need to fix that while we are here. I believe some of the custom lowering happens because of that.

================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1
@@ -1,2 +1,2 @@
-//===-- llvm/CodeGen/GlobalISel/IRTranslator.cpp - IRTranslator --*- C++ -*-==//
+//===--- lib/CodeGen/GlobalISel/IRTranslator.cpp - IRTranslator --*- C++ -*-==//
 //
----------------
Unrelated change.

================
Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:69
@@ -68,2 +68,3 @@
   getMBB().insert(getInsertPt(), NewMI);
+  setInstr(*NewMI, false);
   return NewMI;
----------------
The fact that we were not moving the cursor was actually by design.
If we change that we need to check what is the impact on the existing code.

================
Comment at: lib/CodeGen/LLVMTargetMachine.cpp:163
@@ -162,1 +162,3 @@
 
+    PassConfig->addLegalizeMachineIR();
+
----------------
I’d say we need a addPreLegalize hook as well.


Repository:
  rL LLVM

http://reviews.llvm.org/D21534