[PATCH] D61289: [globalisel] Add G_SEXT_INREG

Thu May 2 08:18:08 PDT 2019

> On May 2, 2019, at 5:18 AM, Diana Picus via Phabricator via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> rovka added inline comments.
> 
> 
> ================
> Comment at: llvm/include/llvm/Target/GenericOpcodes.td:40
> +// returns true) to allow targets to have some bitwidths legal and others
> +// lowered.
> +def G_SEXT_INREG : GenericInstruction {
> ----------------
> dsanders wrote:
>> rovka wrote:
>>> This comment really needs to do a better job explaining the difference between G_SEXT and G_SEXT_INREG. It only covers the mechanical differences (i.e. that you have an immediate operand), but it says nothing about why this different opcode exists or where it would come from. Is the fact that the IRTranslator never creates such instructions relevant? Should we mention that it is only a legalization artifact? Targets can already say that G_SEXT for certain bitwidths is legal, why don't we just allow them to say which bitwidths should be lowered (instead of adding a new opcode)?
>>> This comment really needs to do a better job explaining the difference between G_SEXT and G_SEXT_INREG. It only covers the mechanical
>>> differences (i.e. that you have an immediate operand), but it says nothing about why this different opcode exists or where it would come
>>> from.
>> 
>> Ok I can add to that
>> 
>>> Is the fact that the IRTranslator never creates such instructions relevant?
>> 
>> No, who creates it is irrelevant to the operation of the instruction. There's no guarantee that the legalizer won't receive them as input.
>> 
>> In the case of the IRTranslator, the IRTranslator could create it if it wanted but it's a simple 1:1 converter (for the most part) and chooses not to at the moment as there's no LLVM-IR equivalent. Target-specific passes are also free to create them.
>> 
>>> Should we mention that it is only a legalization artifact?
>> 
>> It's (currently only) created by code that deals with legalization artifacts but it's not a legalization artifact itself.
>> 
>>> Targets can already say that G_SEXT for certain bitwidths is legal, why don't we just allow them to say which bitwidths should be lowered (instead of adding a new opcode)?
>> 
>> It becomes important when you start optimizing with GlobalISel. Suppose that ARM's SXTB instruction has a latency of 1 and and LSL/ASR have a latency of 2 and that this includes forwarding paths in the hardware (if any). Having the signextend as a single atom in the MIR becomes useful for emitting the most efficient code since given code like:
>>  int foo(char a) {
>>    return (int)a << 2;
>>  }
>> it's cheaper to emit:
>>  sxtb r0, r1
>>  lsl r0, r0, #2
>>  // 3 cycles
>> than:
>>  lsl r0, r1, #16
>>  asr r0, r0, #16
>>  lsl r0, r0, #2
>>  // 6 cycles
>> even if you can exploit known-bits to emit:
>>  lsl r0, r1, #16
>>  asr r0, r0, #14
>>  // 4 cycles
>> it would still be better to use the sxtb. The latter example also illustrates that optimization can make it hard to recognise sign-extension. It gets harder if you also reduce the strength of instructions (maybe lsl r0, r0, #1 is faster as add r0, r0, r0) and there's plenty of ways to make things even more difficult. Essentially, the more mangling the optimizer does while ignorant of the desirable code, the harder it is to select the optimal code at the end.
> Sorry, but I still don't get it. I understand why you're trying to avoid the shifts, what I don't understand is why adding this new node is the best solution.
> 
> For one thing, the name is not very descriptive. I guess you just copied it from SelectionDAG, where you can actually constrain the source to match the destination. We don't do that here, so it's just confusing (I mean it sounds as if a legal G_SEXT would be going through memory or something).
> 
> Secondly, it looks like what we need is just a way to tell the artifact combiner "don't turn sext into shifts on this target, for these sizes". Why don't we just use G_SEXT's legality for that? I.e. actually use the regular legality actions on G_SEXT directly instead of G_SEXT_INREG, and tell the combiner to not mess with G_SEXT with legal sizes. With G_SEXT_INREG as proposed in this patch, it looks like you're just moving the type legality problem into a value-of-immediate legality problem for which we need new infrastructure.
> 
> I'm probably missing something, so please bear with me :)

At this point, I have to admit I share Diana’s point of view. So I too missed something :).

> 
> 
> ================
> Comment at: llvm/lib/Target/ARM/ARMLegalizerInfo.cpp:87
> 
> +  getActionDefinitionsBuilder(G_SEXT_INREG).lower();
> +
> ----------------
> Testcase?
> 
> 
> ================
> Comment at: llvm/test/CodeGen/AArch64/GlobalISel/legalize-ext.mir:64
> +    ; CHECK-DAG: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
> +    ; CHECK-DAG: $w0 = COPY [[DEF]](s32)
>     %0:_(s64) = COPY $x0
> ----------------
> dsanders wrote:
>> rovka wrote:
>>> Is the order really irrelevant for all of these? If so, maybe commit just the change from CHECK to CHECK-DAG separately. Personally, I wouldn't mind keeping the CHECK lines so we can see what actually changed with this patch. Ditto for the other tests.
>> The legalizer doesn't provide any guarantees on the order beyond that defs will precede uses. By changing to CHECK-DAG we make the test robust against future changes to the legalizer too. For this patch, the only thing that changed in many cases was the placement of the G_CONSTANT used in the sign-extending shifts
> I looked in more detail and I agree that the order isn't that important. I still think this is an independent change that you can commit before this patch. Keeping it here makes it a bit difficult to spot the tests that are actually relevant for G_SEXT_INREG.
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D61289/new/
> 
> https://reviews.llvm.org/D61289
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits