This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, rs6000][GCC6] Fix PR78543, ICE in push_reload on powerpc64le-linux-gnu
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: bergner at vnet dot ibm dot com (Peter Bergner)
- Cc: gcc-patches at gcc dot gnu dot org (GCC Patches), segher at kernel dot crashing dot org (Segher Boessenkool), dje dot gcc at gmail dot com (David Edelsohn), wschmidt at linux dot vnet dot ibm dot com (Bill Schmidt), meissner at linux dot vnet dot ibm dot com (Michael Meissner), doko at ubuntu dot com (Matthias Klose)
- Date: Tue, 7 Mar 2017 14:34:50 +0100 (CET)
- Subject: Re: [PATCH, rs6000][GCC6] Fix PR78543, ICE in push_reload on powerpc64le-linux-gnu
- Authentication-results: sourceware.org; auth=none
Peter Bergner wrote:
> If we look at rs6000_mode_dependent_address(), it accepts some addresses
> as not being mode dependent:
>
> case PLUS:
> /* Any offset from virtual_stack_vars_rtx and arg_pointer_rtx
> is considered a legitimate address before reload, so there
> are no offset restrictions in that case. Note that this
> condition is safe in strict mode because any address involving
> virtual_stack_vars_rtx or arg_pointer_rtx would already have
> been rejected as illegitimate. */
> if (XEXP (addr, 0) != virtual_stack_vars_rtx
> && XEXP (addr, 0) != arg_pointer_rtx
> && GET_CODE (XEXP (addr, 1)) == CONST_INT)
> {
> unsigned HOST_WIDE_INT val = INTVAL (XEXP (addr, 1));
> return val + 0x8000 >= 0x10000 - (TARGET_POWERPC64 ? 8 : 12);
> }
> break;
>
> It seems wrong that we accept addresses based off virtual_stack_vars_rtx
> and arg_pointer_rtx, but not stack_pointer_rtx (ie, r1) like we have in
> these test cases.
This check is usually done in legitimate_address_p, and there this
distinction is in fact correct: the problem is that when rewriting
an address based on a virtual register into one based on the stack
(or frame) pointer, the offset will change, and may change significantly.
(E.g. you may end up with "frame size - original offset".)
This means that checking the offset relative to the virtual register
is completely pointless:
(A) If the offset relative to the virtual register is in range, the
resulting offset relative to the stack pointer may be out of range.
(B) If the offset relative to the virtual register is out of range,
the resulting offset may still end up *in* range.
Because of (A), reload will re-check offsets after eliminating
registers, and fix up offsets that are now out of range. But
because if (B), if we were to reject out-of-range offsets relative
to a virtual register already in legitimate_address_p, we'd already
commit to generatcwinge more complex code (e.g. precomputing the address)
and even if the final offset would be in range, nobody would then
clean up that unnecessary code any more.
That is why the legitimate_address_p deliberately and correctly
accepts *any* offset for virtual (and eliminable) registers,
thereby deferring the offset validity check until after the
register has been replaced by a real register. But for any
actually real register (like the stack pointer), of course we
*do* have to perform the precise check -- nobody would do that
check otherwise, and we'd end up with invalid instructions.
Now, here we are in mode_dependent_address, which in general
should return true if legitimate_address_p on this address might
return different results depending on the mode of the address.
And in fact, for stack-pointer based addresses, legitimate_address_p
*can* return different results depending on the mode, as the comment
in front of mode_dependent_address explains:
/* Go to LABEL if ADDR (a legitimate address expression)
has an effect that depends on the machine mode it is used for.
On the RS/6000 this is true of all integral offsets (since AltiVec
and VSX modes don't allow them) or is a pre-increment or decrement.
On the other hand, for addresses based on a virtual register,
legitimate_address_p does not depend on the mode since those
are special-cased to be always accepted (see the discussion above).
So I'm not sure that the proposed change is in fact correct ...
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
Ulrich.Weigand@de.ibm.com