On 02/27/2018 04:18 PM, Alexandre Oliva wrote: > On Feb 14, 2018, Jeff Law <l...@redhat.com> wrote: > >>> + regno = REGNO (inc_insn.reg0); >>> + int luid = DF_INSN_LUID (mem_insn.insn); >>> + mem_insn.insn = get_next_ref (regno, bb, reg_next_use); >> So I think a comment is warranted right as we enter the TRUE arm. > >> At that point INC_INSN is an inc/dec. But MEM_INSN is not necessarily a >> memory reference. It could be a memory reference, it could be a copy, >> it could be something completely different (it's just the next insn that >> references the result of the increment). In the case we care about we >> want it to be a copy of INC_INSN's REG_RES back to REG0. > >> ISTM that verifying MEM_INSN is a reg->reg copy (reg_res -> reg0) before >> we call get_next_ref for reg0 is advisable and probably good from a >> compile-time standpoint by avoiding calls into find_address. > > But we don't need it to be a copy. The transformation is just as > legitimate if the regs go independent ways after that point. We have > reg_res set to reg0+reg1, and then a use of reg0 in a MEM before any > other use of reg_res. We turn that into a copy of reg0 to reg_res, and > the MEM addr into a post_add of reg_res with reg1 (possibly a post_inc), > so that the MEM dereferences reg_res while it's still equal to reg0, and > after the MEM, reg_res becomes reg0+reg1, as it should for any > subsequent uses, and reg0 is unmodified. Whether or not a subsequent > copy from reg_res to reg0 is to be found won't make the transformation > any more or less legitimate. > >> After we call get_next_ref to get the next reference of the source of >> the increment, then we're hoping to find a memory reference that uses >> REG0. But it's not guaranteed it's a memory reference insn. > > Yeah, find_address will determine if it contains any of the MEM patterns > we might be interested in, but it could be anything whatsoever. The MEM > pattern might appear virtually anywhere in the insn. > >> I was having an awful time understanding how this code could work from >> the comments until I put it under a debugger and got a sense of the >> state as we entered that IF block. Then it was much clearer :-) > > Sorry, I realize the comments were written based on a lot of context > about the overall behavior of the pass, that I had learned while trying > to figure it out. At the risk of making it redundant, I've expanded the > comments, and added further tests that won't affect current behavior in > any significant way, but that might speed things up a bit and will save > us trouble should find_address be extended to catch additional patterns. > > >> I believe Georg had other testcases in subsequent comments in the BZ, >> but I don't believe they were flagged as regressions. > > However, with the testcases I realized the incremented register could > still be live, even if we didn't find a subsequent use for it. > Adjusting for that made those testcases use post_inc too. > > Here's the improved patch, regstrapped on aarch64-, ppc64-, and > ppc64el-linux-gnu. Ok to install? > > > [PR81611] turn inc-and-use-of-dead-orig into auto-inc > > When the addressing modes available on the machine don't allow offsets > in addresses, odds are that post-increments will be represented in > trees and RTL as: > > y <= x + 1 > ... *(x) ... > x <= y > > so deal with it by turning such RTL as: > > (set y (plus x n)) > ... (mem x) ... > > without intervening uses of y into > > (set y x) > ... (mem (post_add y n)) ... > > so as to create auto-inc addresses that we'd otherwise miss. > > > for gcc/ChangeLog > > PR rtl-optimization/81611 > * auto-inc-dec.c (attempt_change): Move dead note from > mem_insn if it's the next use of regno > (find_address): Take address use of reg holding > non-incremented value. Add parm to limit search to the named > reg only. > (merge_in_block): Attempt to use a mem insn that is the next > use of the original regno. OK. Thanks!
Jeff
