[PATCH] D23253: [X86] Generalized transformation of `definstr gr8; movzx gr32, gr8` to `xor gr32, gr32; definstr gr8`

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 23 12:06:58 PDT 2016


> On Aug 22, 2016, at 7:16 PM, bryant <3.14472+reviews.llvm.org at gmail.com> wrote:
> 
> bryant added inline comments.
> 
> ================
> Comment at: lib/CodeGen/CalcSpillWeights.cpp:46-48
> @@ -45,5 +45,5 @@
> // Return the preferred allocation register for reg, given a COPY instruction.
> -static unsigned copyHint(const MachineInstr *mi, unsigned reg,
> -                         const TargetRegisterInfo &tri,
> -                         const MachineRegisterInfo &mri) {
> +unsigned VirtRegAuxInfo::copyHint(const MachineInstr *mi, unsigned reg,
> +                                  const TargetRegisterInfo &tri,
> +                                  const MachineRegisterInfo &mri) {
>   unsigned sub, hreg, hsub;
> ----------------
> bryant wrote:
>> qcolombet wrote:
>>> This seems wrong to export this interface.
>>> External users should rely on MachineRegisterInfo::getSimpleHint.
>>> 
>>> What is the problem in using MachineRegisterInfo::getSimpleHint?
>> In cases where there is more than one hint, getSimpleHint only returns one and misses the rest.
> Here is an example:
> 
> ```c
> #include <stdint.h>
> 
> typedef struct {
>  uint32_t low;
>  uint32_t high;
> } u32pair;
> 
> extern void use8(uint8_t);
> 
> uint32_t f(uint64_t *m, uint32_t a, uint32_t b) {
>  u32pair src = {a > 0, b > 0};
>  use8(a > 0);
>  u32pair rv;
>  asm volatile("cmpxchg8b %2"
>               : "+a"(rv.low), "+d"(rv.high), "+m"(m)
>               : "b"(src.low), "c"(src.high)
>               : "flags");
>  return rv.low > 0 && rv.high > 0;
> }
> ```
> 
> pre-regalloc machineinstrs:
> 
> ```
> 0B      BB#0: derived from LLVM BB %3
>            Live Ins: %RDI %ESI %EDX
> 16B             %vreg2<def> = COPY %EDX; GR32:%vreg2
> 32B             %vreg1<def> = COPY %ESI; GR32:%vreg1
> 48B             %vreg0<def> = COPY %RDI; GR64:%vreg0
> 64B             MOV64mr <fi#0>, 1, %noreg, 0, %noreg, %vreg0; mem:ST8[%4](tbaa=!2) GR64:%vreg0
> 80B             TEST32rr %vreg1, %vreg1, %EFLAGS<imp-def>; GR32:%vreg1
> 96B             %vreg5<def> = SETNEr %EFLAGS<imp-use,kill>; GR8:%vreg5
> 112B            %vreg6<def> = MOVZX32rr8 %vreg5; GR32:%vreg6 GR8:%vreg5
> 128B            TEST32rr %vreg2, %vreg2, %EFLAGS<imp-def>; GR32:%vreg2
> 144B            %vreg7<def> = SETNEr %EFLAGS<imp-use,kill>; GR8:%vreg7
> 160B            %vreg8<def> = MOVZX32rr8 %vreg7; GR32:%vreg8 GR8:%vreg7
> 176B            ADJCALLSTACKDOWN64 0, 0, %RSP<imp-def,dead>, %EFLAGS<imp-def,dead>, %RSP<imp-use>
> 192B   ==>      %EDI<def> = COPY %vreg6; GR32:%vreg6
> 208B            CALL64pcrel32 <ga:@use8>, <regmask %BH %BL %BP %BPL %BX %EBP %EBX %RBP %RBX %R12 %R13 %R14 %R15 %R12B %R13B %R14B %R15B %R12D %R13D %R14D %R15D %R12W %R13W %R14W %R15W>, %RSP<imp-use>, %EDI<imp-use>, %RSP<imp-def>
> 224B            ADJCALLSTACKUP64 0, 0, %RSP<imp-def,dead>, %EFLAGS<imp-def,dead>, %RSP<imp-use>
> 240B   ==>      %EBX<def> = COPY %vreg6; GR32:%vreg6
> 256B            %ECX<def> = COPY %vreg8; GR32:%vreg8
> 272B            INLINEASM <es:cmpxchg8b $2> [sideeffect] [mayload] [maystore] [attdialect], $0:[regdef], %EAX<imp-def,tied>, $1:[regdef], %EDX<imp-def,tied>, $2:[mem:m], <fi#0>, 1, %noreg, 0, %noreg, $3:[reguse], %EBX<kill>, $4:[reguse], %ECX<kill>, $5:[reguse tiedto:$0], %EAX<undef,tied3>, $6:[reguse tiedto:$1], %EDX<undef,tied5>, $7:[mem:m], <fi#0>, 1, %noreg, 0, %noreg, $8:[clobber], %EFLAGS<earlyclobber,imp-def,dead>, $9:[clobber], %EFLAGS<earlyclobber,imp-def,dead>, <!5>
> 276B            %vreg12<def> = COPY %EDX; GR32:%vreg12
> 280B            %vreg11<def> = COPY %EAX; GR32:%vreg11
> 320B            TEST32rr %vreg11, %vreg11, %EFLAGS<imp-def>; GR32:%vreg11
> 336B            %vreg13<def> = SETNEr %EFLAGS<imp-use,kill>; GR8:%vreg13
> 352B            TEST32rr %vreg12, %vreg12, %EFLAGS<imp-def>; GR32:%vreg12
> 368B            %vreg15<def> = SETNEr %EFLAGS<imp-use,kill>; GR8:%vreg15
> 400B            %vreg15<def,tied1> = AND8rr %vreg15<tied0>, %vreg13, %EFLAGS<imp-def,dead>; GR8:%vreg15,%vreg13
> 416B            %vreg16<def> = MOVZX32rr8 %vreg15; GR32:%vreg16 GR8:%vreg15
> 432B            %EAX<def> = COPY %vreg16; GR32:%vreg16
> 448B            RET 0, %EAX
> ```
> 
> %vreg6 has two hints: EDI and EBX. However, getSimpleHint shows that
> MachineRegisterInfo only records EDI:

That’s what I said, we model only one hint and the getSimpleHint records the most interesting one for you.
The proper way to go with those biases would be to have an ordered list of all the possibilities. That being said, I don’t think we want to introduce that for that patch.
Now, the problem with the reuse of copyHint is that you bias the decision on this one local copy and in that sense this is not a hint. Indeed, by using that color, we may prevent the coalescing of other, more extensive, copies.

The bottom line is although you can find cases where this is going to improve, you could build case it will harm. I am fine sharing the logic of copyHint somewhere, but we need a different name, e.g., getBiasFromCopy.

> 
> ```
> selectOrSplit GR32:%vreg6 [112r,240r:0)  0 at 112r w=5.738636e-03
> hints: %EDI
> missed hint %EDI
> ```
> 
> 
> Repository:
>  rL LLVM
> 
> https://reviews.llvm.org/D23253
> 
> 
> 



More information about the llvm-commits mailing list