Re: Painful problems with -fpic implementation on powerpc-sysv

[Geoff Keating <geoffk at ozemail dot com dot au>]

> Cc: egcs@cygnus.com, Franz.Sirl-kernel@lauterbach.com, law@cygnus.com
> Date: Fri, 21 Aug 1998 11:27:39 -0400
> From: David Edelsohn <dje@watson.ibm.com>
> X-UIDL: 9c6f676aff83562f25614b7bea1441f5
> 
> >>>>> Geoff Keating writes:
> 
> Geoff> Yes.  My patch (attached to that e-mail message) `fixes' the problem
> Geoff> in a really ugly way by forcing egcs to use the stack, not the GOT.
> 
> Geoff> That is what my patch does: it forces reload to try some other
> Geoff> alternative.  My patch has to add an extra hook to force this; H.J
> Geoff> suggested `CONST_DOUBLE_OK_FOR_LETTER_P' but that seems to be
> Geoff> something else (the name is good, but it isn't called when the letter
> Geoff> is 'm' or 'o' :-).  I don't dare to change the meaning of something
> Geoff> like that because it would surely break other ports.
> 
> 	I wouldn't have thought that changes to reload.c itself were
> necessary to accomplish that.  The PowerPC port already materializes FP
> constants in GPRs and moves them through the stack. 
...
> 	There are just so many hooks already in place to tell the backend
> that it needs an additional stack slot at a late stage, that I am
> surprised the changes to reload.c are necessary.

That's why it's really ugly :-).

The problem is not that reload can't load the value using GPRs; it
can, my patch relies on it (well, there was a small bug that had to be
fixed).  It's just that reload _won't_, because it is less efficient.

Reload sees that its choices are either:

- load immediate value in GPRs, store value to memory, load FPR back
  from memory;  or
- load FPR directly from memory.

Naturally, it chooses the second.


I have discovered another case when reload can generate new symbol_ref
references.  Consider the attached test case, compiled with '-O -fpic'.

After CSE and local register allocation, doit() looks like this:

;; Start of basic block 0, registers live: 1 [1] 3 [3] 4 [4] 5 [5] 6 [6] 31 [31]
 93
(insn 238 2 4 (set (reg:SI 128)
        (unspec[ 
                (const_int 0)
            ]  7)) 512 {init_v4_pic} (nil)
    (nil))

...  [lots of unrelated stuff.]

(insn 284 281 286 (set (reg:SI 117)
        (unspec[ 
                (symbol_ref:SI ("@h_malloc"))
                (reg:SI 128)
            ]  8)) 398 {*movsi_got_internal} (insn_list 238 (nil))
    (expr_list:REG_DEAD (reg:SI 128)
        (expr_list:REG_EQUIV (symbol_ref:SI ("@h_malloc"))
            (nil))))

... [three more insns]

;; End of basic block 0

(note 50 289 56 "" NOTE_INSN_LOOP_BEG)

;; Start of basic block 1, registers live: 1 [1] 31 [31] 81 82 83 84 86 88 89 91
 92 93 105 117 119 121 127

[the loop continues, using register 117.  CSE (or something) has
hoisted the load of h_malloc's address out of the loop.]

...

(insn 160 158 162 (set (reg:SI 120)
        (mem:SI (reg:SI 117))) 402 {movsi+1} (nil)
    (nil))

(insn 162 160 164 (set (reg:SI 3 r3)
        (ashift:SI (reg:SI 119)
            (reg/v:SI 81))) 179 {ashlsi3_no_power} (nil)
    (nil))

(call_insn 164 162 166 (parallel[ 
            (set (reg:SI 3 r3)
                (call (mem:SI (reg:SI 120))
                    (const_int 0)))
            (use (const_int 0))
            (clobber (scratch:SI))
        ] ) 497 {call_value_indirect_sysv} (insn_list 160 (insn_list 162 (nil)))
    (expr_list:REG_DEAD (reg:SI 120)
        (expr_list:REG_UNUSED (scratch:SI)
            (nil)))
    (expr_list (use (reg:SI 3 r3))
        (nil)))

[this is the call to *h_malloc.  It is the only other use of pseudo
117.]

Local alloc has decided to put pseudo 128 in hard reg 26---it can do
this because pseudo 128 is used only in block 0.

Now, pseudo 117 doesn't get a register in global alloc initially.  So
reload sees that register 117 is REG_EQUIV to
(symbol_ref ("@h_malloc")), and tries using that directly; this is
what it eventually decides to do:

;; Start of basic block 8, registers live: 1 [1] 81 82 83 84 85 86 87
88 89 91 93 103 105 107 117 119 121 125 127
...
(insn 297 158 160 (set (reg:SI 10 r10)
        (unspec[ 
                (symbol_ref:SI ("@h_malloc"))
                (reg:SI 26 r26)
            ]  8)) 398 {*movsi_got_internal} (nil)
    (nil))

(insn:HI 160 297 162 (set (reg:SI 0 r0)
        (mem:SI (reg:SI 10 r10))) 402 {movsi+1} (nil)
    (nil))
...
(insn 300 162 164 (set (reg:SI 65 lr)
        (reg:SI 0 r0)) 402 {movsi+1} (nil)
    (nil))

(call_insn:HI 164 300 166 (parallel[ 
            (set (reg:SI 3 r3)
                (call (mem:SI (reg:SI 65 lr))
                    (const_int 0)))
            (use (const_int 0))
            (clobber (reg:SI 65 lr))
        ] ) 497 {call_value_indirect_sysv} (insn_list 160 (insn_list 162 (nil)))
    (expr_list:REG_DEAD (reg:SI 0 r0)
        (expr_list:REG_UNUSED (reg:SI 65 lr)
            (nil)))
    (expr_list (use (reg:SI 3 r3))
        (nil)))


But look!  Insn 297 is now using register 26, which is only valid in
basic block 0.  Unfortunately, register 26 was allocated to a
different pseudo before reload by global alloc (it's the loop counter
'a'), and the generated code is bogus.

Now, like the const_double in the earlier case, the REG_EQUIV note is
correct; it's just that reload shouldn't be using it---and, before
anyone mentions it, I tried using LEGITIMATE_PIC_OPERAND_P, but it
does too much.  It's pretty easy to add an extra macro for reload to
also prevent this case, but I'd like to try to find a general solution
that convinces me I haven't missed yet another case.

[The example is from kaffe-1.0b1, but it turns out that linuxthreads
in glibc also suffers from this.]

-- 
Geoffrey Keating <geoffk@ozemail.com.au>

===File ~/gcc-bugs/test11.c=================================
typedef struct _huft {
  unsigned 	e;
  struct _huft* t;
} huft;

huft*   (*h_malloc)(unsigned);

static void
doit( unsigned p2, unsigned p3, huft *p4, huft** t)
{
  unsigned a, h, j, k, w;
  unsigned *xp, *l;
  unsigned lx[2*7+1], v[2*2];
  huft *q;
  huft r;
  huft *u[2];

  memset(v, 0, sizeof(v));
  memset(lx, 0, sizeof(lx));

  w = 0;
  q = 0;
  h = 0;
  l = lx+1;
  
  for (k=1; k <= 2; k++)
    for (a=0; a < 7; a++)
    {
      while (k > w)
      {
        w = l[h++];   

	j = w;
        if (w > a + 1)
        {
          xp = v + k;
          while (j < 2 && w <= xp[j])
	    ++j;
        }
        l[h] = j;        

        q = h_malloc(1<<p2);
        *t = q;
        t = &(q->t);
	*t = 0;
	u[k-1] = q;
      }

      r.e = p3;
      r.t = p4;
      q[0] = r;
    }
}

huft *is_ok(unsigned x) 
{
  exit(0);
}

int main() { h_malloc = is_ok; doit(0, 0, 0, 0); abort(); }

 
============================================================