Re: [PATCH] Sign extension elimination, new implementation, part 1/N

[Leehod Baruch <LEEHOD at il dot ibm dot com>]

Thanks a lot for the review and the useful comments!

> >      - If the instruction has an implicit extension - meaning that
> >        it's high part is an extension of it's low part, mark it as
> >        EXTENDED_DEF and set it's source_mode to be the narrowest
> >        mode that the instruction's low part can fit into.
> 
> This was confusing to me until I looked through the code and
> saw that you actually meant "constant".  It would be better 
> to rename this to CONSTANT_DEF to reflect that.

Currently, I handle only constants but this is just a private case of
EXTENDED_DEF.  For example, the definition can be an instruction that
was previously merged with an extension, like load + extension, and this
merged instruction is not followed by another explicit extension.  It
has an implicit extension but it is not a constant.


> What happens to ref_copy at this point?  It appears to be unused
> afterward.  Surely this cannot be correct.  I suspect that you
> should be using verify_changes, or something of that ilk instead
> of using copy_rtx and replace_regs.

You are right, I forgot to store the manipulated instruction.
I only printed it to the dump file.  But what you say here, if I
understand correctly, is that you suspect that there is a better way
for this merge.
I couldn't find a better way to replace the definition of a register
in an instruction with an expression.
The only function that I could find that does the separation between
definitions and uses is replace_regs.
The manipulation that I did was successful, but I wonder if there
is a different way to do this.

> >      emit_insn (gen_rtx_SET (VOIDmode, subreg_reg, dest_reg));
> 
> If these are truely regs, then you should be using emit_move_insn.
> But then...

I see now that the name "subreg_reg" is confusing, this is an expression
that looks like this: (subreg:Xmode (reg:Ymode)).
Can I use emit_move_insn in this case or do I need to generate and
recognize this set?

> 
> >       if (recog (PATTERN (move_insn), move_insn, NULL) < 0)
> 
> (1) recog does not work on sequences.

Since this sequence has only one instruction, move_insn is the pointer
to it.  I didn't try to recog a sequence.  Is there a better way to
recognize an expression than emitting it to a sequence and recognizing 
the instruction?

> >   /* a. Try to eliminate only the definitions extensions, one by one. 
*/
> >   if (unmerged_def_se_hash)
> >     htab_traverse (((struct see_ref_s *) (stn->value))
> ->unmerged_def_se_hash,
> >          see_merge_one_def_extension, (PTR) (stn->value));
> > 
> >   if (unmerged_def_se_hash && dump_file)
> >     {
> >       /* For debug purpose only.  */
> >       fprintf (dump_file, "unmerged_def_se_hash:\n");
> >       htab_traverse (((struct see_ref_s *) (stn->value))
> ->unmerged_def_se_hash,
> >            see_print_one_extension, NULL);
> >     }
> > 
> >   if (use_se_hash)
> >     {
> >       /* b. Try to eliminate only the uses extensions, one by one.  */
> >       htab_traverse (((struct see_ref_s *) (stn->value))->use_se_hash,
> >            see_merge_one_use_extension, (PTR) (stn->value));
> > 
> >       /* c. Try to eliminate any couple of uses extensions 
> simultaniously.  */
> >       htab_traverse (((struct see_ref_s *) (stn->value))->use_se_hash,
> >            see_merge_two_use_extensions, (PTR) (stn->value));
> 
> At what point are you deciding that A, B, or C has succeeded?  It
> appears to me that you're trying them all always.  Which is of course
> a bit silly.

Well, I do try them all but they are all based on the same hashes.
For instance, if a use extension is merged in B, it will be removed
from the hash and won't be handled again in C.
This way, simple merges are tried before the complicated ones.
Simple merges are merges of one extension (def or use), and complicated
ones are merges of more then one (e.g. two uses simultaneously).

> 
> >   b. If the PROMOTE_MODE definition is set not promote quantities
> >      beyond SImode unless they are actually used in DImode, then at
> >      this point we need to replace every use of the SImode registers
> >      with the proper subreg of the DImode register.
> >      Another, easier, solution is to set the PROMOTE_MODE on all
> >      machines to use subregs of the DImode registers in the first 
> >      place instead of using SImode registers, like it is currently 
> >      done on ppc.
> 
> This doesn't make any sense.  PROMOTE_MODE has nothing to do with
> whether or not you can use a subreg of a DImode register.  The only
> thing that's relevant about PROMOTE_MODE in this conversation at
> all, as far as I can see, is that in some cases it can avoid creating
> the problem that this pass solves.
> 

I'm afraid that our communication about the PROMOTE_MODE is really not
so good :-)

I agree with what you say about it. 
But there is this issue -

You sent me this typical case on alpha:

>    (set (reg:si 100) (mem:si))
>    (set (reg:di 101) (sign_extend:di (reg:si 100)))
>    ...
>    (use (reg:si 100))
>    (use (reg:di 101))

When the see pass is done it will become:

    (set (subreg:si (reg:di 101)) (mem:si))
    ...
    (use (reg:si 100))
    (use (reg:di 101))

and this is why I need to switch every use of (reg:si 100) with a use of
(subreg (reg:si 101)).
This is possible, but it complicates things.
If (reg:si 100) was only a temporary register and every use was of
(subreg (reg:di 101)) in the first place, like it is done in pcc it would 
be much easier.

Do you think that it is reasonable to set the PROMOTE_MODE like it is
on ppc on all machines if the see optimization is on?


Thanks again,
Leehod.