alpha himode reload problem

alpha himode reload problem

[Richard Henderson]

The following program generates bogus rtl in reload on an Alpha:

   struct asdf {
     int space;
     short x;
   };
   
   struct asdf * blah();
   void foo(a,b,c,d,e,f,g)
      int a,b,c,d,e,f,g;
   {
     struct asdf *x = blah();
     while (a < b)
       bar(a++,b,c,d,e,f,g);
     asm volatile("# %0" : : "r"(x->x));
   }

The output from reload is

   (insn 85 69 86 (set (reg:DI 2 $2)
           (mem:DI (plus:DI (reg:DI 30 $30)
                   (const_int 80)))) 252 {movdi-1} (nil)
       (nil))
   
   (insn 86 85 87 (set (reg:SI 2 $2)
           (mem/s:SI (plus:DI (mem:DI (plus:DI (reg:DI 30 $30)
                           (const_int 80)))
                   (const_int 4)))) 244 {movdf+1} (nil)
       (nil))
   
   (insn 87 86 71 (set (subreg:DI (reg:HI 2 $2) 0)
           (zero_extract:DI (subreg:DI (reg:SI 2 $2) 0)
               (const_int 16)
               (const_int 0))) 78 {unaligned_extendhidi+1} (nil)
       (nil))

with the bogosity of course being in insn 86.

The problem seems to me with the fact that reload records replacement
addresses within the rtl, rather than substituting on a pattern; ref
reload.c 3713 (find_reloads):

  /* If this insn pattern contains any MATCH_DUP's, make sure that
     they will be substituted if the operands they match are substituted.
     Also do now any substitutions we already did on the operands.

and alpha.md 4179 (movhi):

      if (aligned_memory_operand (operands[1], HImode))
        {
          rtx aligned_mem, bitnum;
          rtx scratch = (reload_in_progress
                         ? gen_rtx (REG, SImode, REGNO (operands[0]))
                         : gen_reg_rtx (SImode));

          get_aligned_mem (operands[1], &aligned_mem, &bitnum);

          emit_insn (gen_aligned_loadhi (operands[0], aligned_mem, bitnum,
                                         scratch));
        }

The movhi pattern itself contains no match_dup, so no additional 
replacements are generated.

Any ideas on how to handle this?


r~

Re: alpha himode reload problem

[Jim Wilson]

	The problem seems to me with the fact that reload records replacement
	addresses within the rtl, rather than substituting on a pattern; ref
	reload.c 3713 (find_reloads):

There is not supposed to be any RTL generated during reload, except for
RTL that reload knowingly generates itself, and reload always creates RTL
in such a way that it either doesn't need reloads or will be reloaded
by the replacements.

Since we never create random RTL, we can keep track of the addresses that need
to be stored into, and this is more efficient than searching through the RTL
to looking for stuff after the fact that might need fixing.  This also
means we don't have to worry about ambiguous cases, e.g. for instance if a
particular RTL occurs in multiple places, but only some of which need to
modified.  This is easy to do if we keep track of addresses to do; much harder
if we try to find them via pattern matching.

This breaks down for your example because of two reasons:
- The alpha movhi pattern creates a new MEM rtx because it doesn't support
  HImode loads.
- There is an asm that contains a HImode MEM, and valid asm operands are not
  passed through emit_move_insn.

Normally, all HImode MEMs would get fixed during RTL generation, because
they have to pass through movhi, and movhi will rewrite them then.  HImode
MEMs should only occur during reload as the result of substitution from
REG_EQUIVs, in which case I suspect the address will always be a constant,
or a stack offset, or something else which will not need to be reloaded.

One way to fix this is to modify the movhi pattern to reload the address
itself if necessary, but this isn't very elegant.  If we do adopt this
approach, then we need to fix the mov*i patterns too as necessary.

Another way to fix this is to modify expand_asm_operands to fix the MEM
during RTL generation.  This seems more elegant.  There is already code to
fix a few cases.  We could add a case that handles MEM operands.  Unfortunately
I don't see any obvious easy way to write the test.  The info we need is
encoded in the mov*i patterns.  There isn't any particular test we can
perform to see if the MEM operand needs to be handled differently.  We
could perhaps just call emit_move_insn to move the MEM into a temp reg,
and then check to see if we got more than one instruction, and if so, we emit
all the insns and use the temp reg as the actual operand.  Of course, that
only works if we wanted a reg operand in the first place.  Maybe we need
a flag that indicates whether memory operands are OK, and if not, and we
have a MEM, then we load the MEM into a pseudo and use the pseudo as the
operand.  This is much better than waiting until reload to fix the operand,
as we then have a chance to optimize the MEM.  This would work for your
example.

FYI Here is an ugly hack I wrote to fix the problem in the alpha.md movhi
pattern.  I think the expand_asm_operands fix is much better though.

Index: alpha.md
===================================================================
RCS file: /cvs/cvsfiles/egcs/gcc/config/alpha/alpha.md,v
retrieving revision 1.9
diff -p -r1.9 alpha.md
*** alpha.md	1997/10/20 15:50:28	1.9
--- alpha.md	1997/10/23 01:59:06
***************
*** 4183,4188 ****
--- 4183,4199 ----
  			 ? gen_rtx (REG, SImode, REGNO (operands[0]))
  			 : gen_reg_rtx (SImode));
  
+ 	  /* ??? This code creates a new MEM rtx.  If we were called during
+ 	     reload, then we must be careful to make sure that the new
+ 	     rtx will not need reloading.  */
+ 	  if (reload_in_progress &&
+ 	      ! strict_memory_address_p (SImode, XEXP (operands[1], 0)))
+ 	    {
+ 	      rtx tmp = gen_rtx (REG, Pmode, REGNO (operands[0]));
+ 	      emit_insn (gen_move_insn (tmp, XEXP (operands[1], 0)));
+ 	      XEXP (operands[1], 0) = tmp;
+ 	    }
+ 
  	  get_aligned_mem (operands[1], &aligned_mem, &bitnum);
  
  	  emit_insn (gen_aligned_loadhi (operands[0], aligned_mem, bitnum,


Jim

Re: alpha himode reload problem

[Richard Henderson]

On Wed, Oct 22, 1997 at 07:01:01PM -0700, Jim Wilson wrote:
> Another way to fix this is to modify expand_asm_operands to fix the MEM
> during RTL generation.  This seems more elegant. 

Blah.  Outsmarted by combine.  You can see the results just by changing
the last line of the example to

  { unsigned short tmp = x->x;
    asm volatile ("%0" : : "r"(tmp)); }

Everything gets expand out nicely, but then combine puts it all back 
together again.  I suppose we could add more code there, but the .md
solution is starting to look more attractive...


r~

Re: alpha himode reload problem

[Richard Henderson]

On Wed, Oct 22, 1997 at 07:01:01PM -0700, Jim Wilson wrote:
> Another way to fix this is to modify expand_asm_operands to fix the MEM
> during RTL generation.  This seems more elegant.

Comments on the following?  The recog.c changes were necesary to 
get combine not to undo our hard work in stmt.c.


r~

Re: alpha himode reload problem

[Richard Henderson]

On Sun, Oct 26, 1997 at 04:09:47PM -0800, Richard Henderson wrote:
> Comments on the following?  The recog.c changes were necesary to 
> get combine not to undo our hard work in stmt.c.

Blah.  I should have tested this on something real like the kernel.
It needs a bit more thought, but comments on what ought to happen
are still welcome.

In the meantime, the attached patch fills out the brute force solution
Jim posted, for 8 cases (also attached) I could think of.  Any other
points that are likely to be called?

At least the kernel builds now.  We'll see how well it boots in a 
moment.  ;-)


r~

Re: alpha himode reload problem

[Jim Wilson]

	Comments on the following?  The recog.c changes were necesary to 
	get combine not to undo our hard work in stmt.c.

It is annoying that combine undoes the work that the mov*i patterns did.
Since we are already using general_operand (via check_asm_operands) perhaps
the right fix is to somehow make general_operand reject the QImode/HImode MEMs
that aren't supported on some alphas?  This may even make the stmt.c changes
unnecessary if it is actually workable.  This would make it impossible to
make a QImode/HImode MEM match a "m" constraint in an asm though, so this
might not be an acceptable solution.

Otherwise, I think you need to be more conservative in the handling of the
extra constraints.  They might be accepting registers, or they might be
accepting memory, or they might be accepting constants.  There is no way to
know.  If the operand does happen to be OK for the constraint, then we don't
want to do anything that will upset what the user intended.  Hence, we must
make the most conservative assumption.  For allow_mem, this means setting
it by default.  Of course, this means that the handling of extra constraints
is still broken, but it is perhaps not worth trying to fix this.

I didn't mention it the first time, but because extended asms are so closely
tied to gcc internals, sometimes it isn't possible to handle all possible
cases, and there is no choice but to ask the user to rewrite their asm.

Jim