Jiufu Guo via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Hi Segher!
>
> Thanks a lot for your comments!
>
> Segher Boessenkool <seg...@kernel.crashing.org> writes:
>
>> Hi guys,
>>
>> On Fri, Nov 25, 2022 at 04:11:49PM +0800, Kewen.Lin wrote:
>>> on 2022/10/26 19:40, Jiufu Guo wrote:
>>> for "li/lis + oris/xoris", I interpreted it into four combinations:
>>>
>>> li + oris, lis + oris, li + xoris, lis + xoris.
>>>
>>> not sure just me interpreting like that, but the actual combinations
>>> which this patch adopts are:
>>>
>>> li + oris, li + xoris, lis + xoris.
>>>
>>> It's a bit off, but not a big deal, up to you to reword it or not. :)
>>
>> The first two are obvious, but the last one is almost never a good idea,
>> there usually are better ways to do the same. I cannot even think of
>> any case where this is best? A lis;rl* is always prefered (it can
>> optimise better, be combined with other insns).
> I understant your point here. The first two: 'li' for lowest 16bits,
> 'oris/xoris' for next 16bits.
>
> While for 'lis + xoris', it may not obvious, because both 'lis' and
> 'xoris' operates on 17-31bits.
> 'lis + xoris' is for case "32(1) || 1(0) || 15(x) || 16(0)". xoris is
> used to clean bit31. This case seems hard to be supported by 'rlxx'.
>
> I hit to find this case when I analyze what kind of constants can be
> build by two instructions. Checked the posssible combinations:
> "addi/addis" + "neg/ori/../xoris/rldX/rlwX/../sradi/extswsli"(those
> instructions which accept one register and one immediate).
>
> I also drafted the patch to use "li/lis+rlxx" to build constant.
> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/601276.html
> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/601277.html
>
>>
>>> > + HOST_WIDE_INT orig_c = c;
>>
>> If you ever feel you need a variable to hold an "orig" value, that is a
>> good hint that you should restructure the code a bit, perhaps even
>> factor it. That often is overdue (like here), not caused by you, but
>> you could help solve it ;-)
>>
>> (This is what made this patch hard to review, btw).
> You are right. Thanks for point out this!
>>
>>> > gen_rtx_IOR (DImode, copy_rtx (temp),
>>> > GEN_INT (ud1)));
>>> > }
>>> > + else if ((ud4 == 0xffff && ud3 == 0xffff)
>>> > + && ((ud1 & 0x8000) || (ud1 == 0 && !(ud2 & 0x8000))))
>>> > + {
>>> > + temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>>> > +
>>> > + HOST_WIDE_INT imm = (ud1 & 0x8000) ? ((ud1 ^ 0x8000) - 0x8000)
>>> > + : ((ud2 << 16) - 0x80000000);
>>
>> We really should have some "hwi::sign_extend (ud1, 16)" helper function,
>> heh. Maybe there already is? Ah, "sext_hwi". Fixing that up
>> everywhere in this function is preapproved.
> Great, thanks!
>>
>>> > + else
>>> > + {
>>> > + emit_move_insn (temp,
>>> > + GEN_INT (((ud2 << 16) ^ 0x80000000) - 0x80000000));
>>> > + if (ud1 != 0)
>>> > + emit_move_insn (temp, gen_rtx_IOR (DImode, temp, GEN_INT (ud1)));
>>> > + emit_move_insn (dest,
>>> > + gen_rtx_ZERO_EXTEND (DImode,
>>> > + gen_lowpart (SImode, temp)));
>>> > + }
>>
>> Why this? Please just write it in DImode, do not go via SImode?
> Thanks for catch this. Yes, gen_lowpart with DImode would be ok.
Oh, Sorry. DImode can not be used here. The genreated pattern with
DImode can not be recognized. Using SImode is to match 'rlwxx'.
BR,
Jeff (Jiufu)
>>
>>> > --- /dev/null
>>> > +++ b/gcc/testsuite/gcc.target/powerpc/pr106708.h
>>> > @@ -0,0 +1,9 @@
>>> > +/* Test constants which can be built by li/lis + oris/xoris */
>>> > +void __attribute__ ((__noinline__, __noclone__)) foo (long long *arg)
>>> > +{
>>> > + *arg++ = 0x98765432ULL;
>>> > + *arg++ = 0xffffffff7cdeab55ULL;
>>> > + *arg++ = 0xffffffff65430000ULL;
>>> > +}
>>
>> Use noipa please (it is shorter, simpler, and covers more :-) )
> Thanks!
>>
>> Could you comment what exact instructions are expected?
>> li;xoris and li;xoris and lis;xoris I guess? It helps if you just tell
>> the reader here.
> Sure, thanks!
>>
>> The li;oris and li;xoris parts look good.
>>
>
> BR,
> Jeff (Jiufu)
>>
>> Segher
