Hi, Bernd

2016-05-13 20:33 GMT+08:00 Bernd Schmidt <bschm...@redhat.com>:
> On 05/05/2016 08:03 AM, Shiva Chen wrote:
>>
>>
>> -  /* We do not handle setting only part of the register.  */
>> -  if (DF_REF_FLAGS (adef) & DF_REF_READ_WRITE)
>> -    return GRD_INVALID;
>> -
> This isn't right, at least not without other changes. This prevents using
> references where the register is set as part of a ZERO_EXTRACT or
> STRICT_LOW_PART, and you seem not to handle such cases in the new code.
>
> I think this interface is pretty bad. I think what you should do is check in
> the caller whether the def is of READ_WRITE type. If so, do a
> FOR_EACH_SUBRTX thing to look for a memory reference containing an autoinc
> expression, otherwise this new function (without the MEM parts) could be
> used. That'll also solve the problem above with the loss of
> DF_REF_READ_WRITE checking.
>
> The walk to find the appropriate MEM would be in a separate function, used
> in the several places that

Yes, the interface is bad.
I split the function into read_write_reg_mem and get_reg_calculate_expr.
The function read_write_reg_mem try to find autoinc in memory
reference by FOR_EACH_SUBRTX if the def is READ_WRITE type which fix
the lost of checking as your suggestion.

>> +  /* Find REG increment/decrement expr in following pattern
>> +
>> +     (parallel
>> +           (CC = compare (REG - 1, 0))
>> +           (REG = REG - 1))
>> +   */
>
>
> Comment formatting, "*/" doesn't go on its own line. I think it's best not
> to quote a pattern here since you're not actually looking for it.  A better
> comment would be "For identifying ivs, we can actually ignore any other SETs
> occurring in parallel, so look for one with the correct SET_DEST."  I'm not
> actually sure, however, whether this is valid. It would depend on how this
> information is used later on, and what transformation other passes would
> want to do on the ivs.  Come to think of it, this is also true for autoinc
> ivs, but I guess these would only appear when run late as analysis for
> modulo-sched, so I guess that would be OK?
Thanks for the correctness of the comment.

>> @@ -2354,6 +2489,20 @@ iv_number_of_iterations (struct loop *loop,
>> rtx_insn *insn, rtx condition,
>>       goto fail;
>>
>>     op0 = XEXP (condition, 0);
>> +
>> +  /* We would use loop induction variable analysis
>> +     to get loop iteration count in SMS pass
>> +     which should work with/without doloop pass
>> +     active or not.
>> +
>> +     With doloop pass enabled, doloop pass would
>> +     generate pattern as (ne (REG - 1), 0) and
>> +     we recognize it by following code.  */
>> +  if (GET_CODE (op0) == PLUS
>> +      && CONST_INT_P (XEXP (op0, 1))
>> +      && REG_P (XEXP (op0, 0)))
>> +    op0 = XEXP (op0, 0);
>
>
> That really looks somewhat suspicious, and I can't really tell whether it's
> right. My instinct says no; how can you just drop a constant on the floor?

I think your instinct is right.
op0 and op1 split from loop end condition will through to iv_analyze
and get iv0 and iv1.
The following code will use iv0 and iv1 to estimate loop iteration count.
I think the correct think to do is transfer  (ne (REG - 1), 0)  to (ne
REG, 1) which means setting op0 = REG, op1 = 1.
Then iv1.base could set correctly by iv_analyze.

Thanks for your comments, they're truly helpful.
please correct me if i still missing something.

Shiva

Attachment: 0007-To-identify-READ_WRITE_REG-as-loop-induction-variable-v1.patch
Description: Binary data

Reply via email to