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

[Quentin Colombet via llvm-commits]

> On Jul 7, 2016, at 2:18 PM, Tim Northover <t.p.northover at gmail.com> wrote:
> 
> t.p.northover marked 7 inline comments as done.
> t.p.northover added a comment.
> 
> I'm not uploading an actual diff yet (I'll want to split out more bits into separate reviews I think). But some comment replies:
> 
> 
> ================
> 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);
> +
> ----------------
> qcolombet wrote:
>> 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
> OK, I'll give it a go. For now I'll limit it to a single (implicit) size since otherwise we'll have to deal with multiple types.

Good point, forgot about those types.
The information would have already being encoded in the size of the registers but yeah, theoretically the generic instructions need types :).

> 
> ================
> Comment at: include/llvm/Target/GenericOpcodes.td:66
> @@ +65,3 @@
> +  let OutOperandList = (outs unknown:$dst, unknown:$carry_out);
> +  let InOperandList = (ins unknown:$src1, unknown:$src2, unknown:$carry_in);
> +  let AsmString = "";
> ----------------
> arsenm wrote:
>> 0 indexed src name? Should named operand tables somehow work with the generic opcodes?
> Existing instructions are indexed by 1. Could do that in a separate refactoring though, I don't particularly feel strongly either way.
> 
> ================
> Comment at: lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:69
> @@ -68,2 +68,3 @@
>   getMBB().insert(getInsertPt(), NewMI);
> +  setInstr(*NewMI, false);
>   return NewMI;
> ----------------
> qcolombet wrote:
>> 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.
> Well, all the tests still pass. ;)
> 
> I think we definitely need to do something so that the default use doesn't end up inserting instructions in reverse order. Maybe make setInstr default to Before = true instead?

I thought that the default was already the true :). So, SGTM.


> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D21534
> 
> 
>