VTA RTL patch review?

[Alexandre Oliva]

On Aug  3, 2009, Richard Henderson <rth@redhat.com> wrote:

>> @@ -265,6 +265,7 @@ regrename_optimize (void)
>> n_uses = 0;
>> for (last = this_du; last->next_use; last = last->next_use)
>> {
>> +             /* ??? Do we need to discount debug insns here?  */
>> n_uses++;

> The answer is yes.  Otherwise we get substitutions that only ocurr
> when debug insns are present.

That would be true if uses in debug insns ever made to the du chains
constructed in this pass.  But they don't.  Comment updated.

>> @@ -819,7 +820,8 @@ build_def_use (basic_block bb)
>> *recog_data.dup_loc[i] = cc0_rtx;
>> }
>> 
>> -         scan_rtx (insn, &PATTERN (insn), NO_REGS, terminate_all_read,
>> +         scan_rtx (insn, &PATTERN (insn), NO_REGS,
>> +                   DEBUG_INSN_P (insn) ? mark_access : terminate_all_read,
>> OP_IN, 0);

> I believe the register class for debug insns will always be NO_REGS,
> which will prevent any renaming.

Err...  Why would it?  mark_access for OP_IN will do nothing in
scan_rtx_reg() and scan_rtx_address(), so scan_rtx will effectively
ignore all regs.

But then, I can't figure out why we even scan debug insns here.  It
seems to me that it doesn't do anything useful.  It appears to me that
running the entire body only if NONDEBUG_INSNS_P wouldn't change the
observable behavior.  n_ops and n_dups are zero for a DEBUG_INSN, so the
per-operand calls do nothing, and the notes aren't relevant for debug
insns, so this call would have done all the work, but it also does
nothing.  I say let's kill it and see how that goes...  However...

As for whether it's correct to refrain from adding uses in debug insns
to the DU chains...  It seemed to be at first, but now I'm getting
convinced it isn't.  do_replace() doesn't change the reg itself, it
overwrites the pointers to it in the chain, so debug info would be left
alone, i.e., out of date.

So, instead of making the changes suggested above, I'll look into fixing
this.  Thanks for drawing my attention to it.

>   if (DEBUG_INSN_P (insn))
>     scan_rtx (insn, &PATTERN (insn), ALL_REGS, mark_access, OP_IN, 0);
>   else
>     scan_rtx (insn, &PATTERN (insn), NO_REGS, terminate_all_read,
>               OP_IN, 0);


>> @@ -409,7 +409,8 @@ get_attr_length_1 (rtx insn ATTRIBUTE_UN
>> 
>> case INSN:
>> body = PATTERN (insn);
>> -       if (GET_CODE (body) == USE || GET_CODE (body) == CLOBBER)
>> +       if (GET_CODE (body) == USE || GET_CODE (body) == CLOBBER
>> +           || DEBUG_INSN_P (insn))

> How could that condition ever be true?

It couldn't.  It was a mechanical change.  Thanks for catching it.

>> @@ -2474,6 +2476,8 @@ check_cond_move_block (basic_block bb, r
>> /* We can only handle simple jumps at the end of the basic block.
>> It is almost impossible to update the CFG otherwise.  */
>> insn = BB_END (bb);
>> +  while (DEBUG_INSN_P (insn))
>> +    insn = PREV_INSN (insn);
>> if (JUMP_P (insn) && !onlyjump_p (insn))

> Surely we don't allow debug insns after a jump, within a
> block. Allowing jump insns to be interior to a BB seems like a very
> bad invariant to break.

I tend to agree.  I *think* I experimented with that and ended up
dropping it, but I'll double-check before posting a consolidated patch
for your reviews.


>> @@ -2559,7 +2563,8 @@ cond_move_convert_if_block (struct noce_
>> rtx set, target, dest, t, e;
>> unsigned int regno;
>> 
>> -      if (!INSN_P (insn) || JUMP_P (insn))
>> +      /* ??? Maybe emit conditional debug insn?  */
>> +      if (!NONDEBUG_INSN_P (insn) || JUMP_P (insn))

> You're allowing a debug insn to affect whether a block is
> conditionalized.

Err, am I?  I don't see how.

> Until you support conditional debug insns, surely you'd prefer to drop
> the debug insn if the block is successfully conditionalized.

That's exactly what I think the code is doing.  If it *is* a DEBUG_INSN,
we'll continue, like we would for a NOTE, and a NOTE certainly doesn't
affect whether a block is conditionalized.

It would be strictly more correct to emit a debug insn unbinding the
variable, at least until conditional debug insns are introduced, but we
don't do that ATM, so debug info may end up incorrect in such
conditional blocks.  It *probably* won't be much of a significant loss:
at least for surviving variables, they will likely share the same
location in both conditional branches, and get back in sync with the
debug note emitted at the PHI node of the confluence point, and in other
cases we probably wouldn't be able to tell which location to choose
anyway.  But there's definitely room for long-term improvement.

>> /* We use validate_replace_rtx, in case there
>> are multiple identical source operands.  All of
>> them have to be changed at the same time.  */
>> +                     validate_change (p, &SET_DEST (pset), dst, 1);
>> if (validate_replace_rtx (src, dst, insn))

> You would do well to update the comment to mention that
> validate_replace_rtx calls apply_change_group, and so this ordering of
> calls ensures that the source and destination substitutions are
> validated simultaneously.

ACK


>> @@ -450,7 +450,10 @@ replace_oldest_value_reg (rtx *loc, enum
>> fprintf (dump_file, "insn %u: replaced reg %u with %u\n",
>> INSN_UID (insn), REGNO (*loc), REGNO (new_rtx));
>> 
>> -      validate_change (insn, loc, new_rtx, 1);
>> +      if (DEBUG_INSN_P (insn))
>> +       *loc = new_rtx;
>> +      else
>> +       validate_change (insn, loc, new_rtx, 1);

> This change is wrong, since it doesn't get backed out if the
> apply_change_group fails.  

Yuck!  Yeah, bad Alex, bad!

> What caused you to need it in the first place?

-ENOIDEA

I'll back it out and find out.  It's most likely a remnant from the very
early days, when DEBUG_INSNs wouldn't pass validation.  I *think* I
fixed that long ago but, if not, now will be a good time ;-)

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer