Re: [PATCH] split loop for NE condition.

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

On 2021-05-01 05:37, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Apr 29, 2021 at 05:50:48PM +0800, Jiufu Guo wrote:
>> When there is the possibility that overflow may happen on the loop 
>> index,
>> a few optimizations would not happen. For example code:
>> 
>> foo (int *a, int *b, unsigned k, unsigned n)
>> {
>>   while (++k != n)
>>     a[k] = b[k]  + 1;
>> }
>> 
>> For this code, if "l > n", overflow may happen.  if "l < n" at 
>> begining,
>> it could be optimized (e.g. vectorization).
> 
> FWIW, this isn't called "overflow" in C: all overflow is undefined
> behaviour.
> 
> "A computation involving unsigned operands can never overflow, because 
> a
> result that cannot be represented by the resulting unsigned integer 
> type
> is reduced modulo the number that is one greater than the largest value
> that can be represented by the resulting type."

Thanks for point out this, yes, it may be better to call it as 'wrap' :)

> 
>> +-param=max-insns-ne-cond-split=
>> +Common Joined UInteger Var(param_max_insn_ne_cond_split) Init(64) 
>> Param Optimization
>> +The maximum threshold for insnstructions number of a loop with ne 
>> condition to split.
> 
> "number of instructions".
> 
> Perhaps you should mark up "ne" as a codeword somehow, but because it
> is in a help text it is probably better to just, write out "not equal"
> or similar?

Would update it accordingly. Thanks for your suggestion!

> 
>> @@ -248,13 +250,14 @@ connect_loop_phis (class loop *loop1, class loop 
>> *loop2, edge new_e)
>>         !gsi_end_p (psi_first);
>>         gsi_next (&psi_first), gsi_next (&psi_second))
>>      {
>> -      tree init, next, new_init;
>> +      tree init, next, new_init, prev;
>>        use_operand_p op;
>>        gphi *phi_first = psi_first.phi ();
>>        gphi *phi_second = psi_second.phi ();
>> 
>>        init = PHI_ARG_DEF_FROM_EDGE (phi_first, firste);
>>        next = PHI_ARG_DEF_FROM_EDGE (phi_first, firstn);
>> +      prev = PHI_RESULT (phi_first);
>>        op = PHI_ARG_DEF_PTR_FROM_EDGE (phi_second, seconde);
>>        gcc_assert (operand_equal_for_phi_arg_p (init, USE_FROM_PTR 
>> (op)));
>> 
> 
> I would just declare it at the first use...  Less mental load for the
> reader.  (And a smaller patch ;-) )
Yeap, thanks!

> 
>> +/* Check if the LOOP exit branch likes "if (idx != bound)".
>> +   if INV is not NULL and the branch is "if (bound != idx)", set *INV 
>> to true.
> 
> "If INV", sentences start with a capital.

Thanks :)
> 
>> +      /* Make sure idx and bound.  */
>> +      tree idx = gimple_cond_lhs (cond);
>> +      tree bnd = gimple_cond_rhs (cond);
>> +      if (expr_invariant_in_loop_p (loop, idx))
>> +	{
>> +	  std::swap (idx, bnd);
>> +	  if (inv)
>> +	    *inv = true;
>> +	}
>> +      else if (!expr_invariant_in_loop_p (loop, bnd))
>> +	continue;
> 
> Make sure idx and bound what?  What about them?
> 
>> +      /* Make sure idx is iv.  */
>> +      class loop *useloop = loop_containing_stmt (cond);
>> +      affine_iv iv;
>> +      if (!simple_iv (loop, useloop, idx, &iv, false))
>> +	continue;
> 
> "Make sure idx is a simple_iv"?
Thanks, the comment should be more clear, the intention is:
make sure "lhs/rhs" pair are "index/bound" pair.

> 
>> +
>> +      /* No need to split loop, if base is know value.
>> +	 Or check range info.  */
> 
> "if base is a known value".  Not sure what you mean with range info?
> A possible future improvement?
The intention is "If there is no wrap/overflow happen", no need to split 
loop".
If the base is a known value, the index may not wrap/overflow and may be 
able
optimized by other passes.
Using range-info to check wrap/overflow could be a future improvement.

> 
>> +      /* There is type conversion on idx(or rhs of idx's def).
>> +	 And there is converting shorter to longer type. */
>> +      tree type = TREE_TYPE (idx);
>> +      if (!INTEGRAL_TYPE_P (type) || TREE_CODE (idx) != SSA_NAME
>> +	  || !TYPE_UNSIGNED (type)
>> +	  || TYPE_PRECISION (type) == TYPE_PRECISION (sizetype))
>> +	continue;
> 
> "IDX is an unsigned type that is widened to SIZETYPE" etc.
This is better wording :)

> 
> This code assumes SIZETYPE is bigger than any other integer type.  Is
> that true?  Even if so, the second comment could be improved.
> 
> (Not reviewing further, my Gimple isn't near good enough, sorry.  But
> at least to my untrained eye it looks pretty good :-) )

Thanks so much for your very helpful comments!

Jiufu Guo.

> 
> 
> Segher