Re: [PATCH V2] Use preferred mode for doloop iv [PR61837].

[guojiufu <guojiufu@linux.ibm.com>]

On 2021-07-14 04:50, Segher Boessenkool wrote:
> Hi!
> 
> On Tue, Jul 13, 2021 at 08:50:46PM +0800, Jiufu Guo wrote:
>> 	* doc/tm.texi: Regenerated.
> 
> Pet peeve: "Regenerate.", no "d".

Ok, yeap. While, 'Regenerate and Regenerated' were used by commits 
somewhere :)

> 
>> +DEFHOOK
>> +(preferred_doloop_mode,
>> + "This hook returns a more preferred mode or the @var{mode} itself.",
>> + machine_mode,
>> + (machine_mode mode),
>> + default_preferred_doloop_mode)
> 
> You need a bit more description here.  What does the value it returns
> mean?  If you want to say "a more preferred mode or the mode itself",
> you should explain what the difference means, too.

Ok, thanks.

> 
> You also should say the hook does not need to test if things will fit,
> since the generic code already does.
> 
> And say this should return a MODE_INT always -- you never test for that
> as far as I can see, but you don't need to, as long as everyone does 
> the
> sane thing.  So just state every hok implementation should :-)

Yes, the preferred 'doloop iv mode' from targets should be a MODE_INT.
I will add comments, and update the gcc_assert you mentioned below
for this.

Thanks a lot for your comments and suggestions!

When writing words, I was always from adding/deleting and still hard to 
get
perfect ones -:(

> 
>> +extern machine_mode
>> +default_preferred_doloop_mode (machine_mode);
> 
> One line please (this is a declaration).
> 
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2" } */
>> +void foo(int *p1, long *p2, int s)
>> +{
>> +  int n, v, i;
>> +
>> +  v = 0;
>> +  for (n = 0; n <= 100; n++) {
>> +     for (i = 0; i < s; i++)
>> +        if (p2[i] == n)
>> +           p1[i] = v;
>> +     v += 88;
>> +  }
>> +}
>> +
>> +/* { dg-final { scan-assembler-not {\mrldicl\M} } } */
> 
> That is a pretty fragile thing to test for.  It also need a line or two
> of comment in the test case what this does, what kind of thing it does
> not want to see.

Thanks! I will update accordingly.  And I'm thinking to add tests to 
check
doloop.xx type: no zero_extend to access subreg. This is the intention
of this patch.

> 
>> +/* If PREFERRED_MODE is suitable and profitable, use the preferred
>> +   PREFERRED_MODE to compute doloop iv base from niter: base = niter 
>> + 1.  */
>> +
>> +static tree
>> +compute_doloop_base_on_mode (machine_mode preferred_mode, tree niter,
>> +			     const widest_int &iterations_max)
>> +{
>> +  tree ntype = TREE_TYPE (niter);
>> +  tree pref_type = lang_hooks.types.type_for_mode (preferred_mode, 
>> 1);
>> +
>> +  gcc_assert (pref_type && TYPE_UNSIGNED (ntype));
> 
> Should that be pref_type instead of ntype?  If not, write it as two
> separate asserts please.

Ok, will separate as two asserts.

> 
>> +static machine_mode
>> +rs6000_preferred_doloop_mode (machine_mode)
>> +{
>> +  return word_mode;
>> +}
> 
> This is fine if the generic code does the right thing if it passes say
> TImode here, and if it never will pass some other mode class mode.

The generic code checks if the returned mode can works on doloop iv 
correctly,
if the preferred mode is not suitable (e.g. preferred_doloop_mode 
returns DI,
but niter is a large value in TI), then doloop.xx IV will use the 
original mode.

When a target really prefer TImode, and TImode can represent number of 
iterations,
This would still work.  In current code, word_mode is SI/DI in most 
targets, like Pmode.
On powerpc, they are DImode (for 64bit)/SImode(32bit)

Thanks again for your comments!

BR,
Jiufu

> 
> 
> Segher