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
0007-To-identify-READ_WRITE_REG-as-loop-induction-variable-v1.patch
Description: Binary data
