* Fix PR rtl-optimization/33822
@ 2007-11-07 20:49 Eric Botcazou
2007-11-09 22:14 ` Richard Sandiford
0 siblings, 1 reply; 10+ messages in thread
From: Eric Botcazou @ 2007-11-07 20:49 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]
This is a regression introduced by the var-tracking pass in the 4.x series.
The pass puts a limit (16) on the number of "location parts" it can track for
each variable. This limit is tested only in track_expr_p, implicitly for
variables in registers (16 is the size of TImode) and explicitly for
variables in memory:
/* Maximum number of location parts. */
#define MAX_VAR_PARTS 16
/* If RTX is a memory it should not be very large (because it would be
an array or struct). */
if (MEM_P (decl_rtl))
{
/* Do not track structures and arrays. */
if (GET_MODE (decl_rtl) == BLKmode
|| AGGREGATE_TYPE_P (TREE_TYPE (realdecl)))
return 0;
if (MEM_SIZE (decl_rtl)
&& INTVAL (MEM_SIZE (decl_rtl)) > MAX_VAR_PARTS)
return 0;
}
But it is asserted in several places later.
This works fine for valid programs, but the assertions can trigger for invalid
programs with out-of-bounds accesses (testcase from Andrew attached). The
patch makes it so that out-of-bounds accesses are not tracked.
Bootstrapped/regtested on x86_64-suse-linux, applied on all active branches.
2007-11-07 Eric Botcazou <ebotcazou@libertysurf.fr>
PR rtl-optimization/33822
* rtl.h (REG_OFFSET): Fix comment.
* var-tracking.c (INT_MEM_OFFSET): New macro.
(var_mem_set): Use it.
(var_mem_delete_and_set): Likewise.
(var_mem_delete): Likewise.
(same_variable_part_p): Likewise.
(vt_get_decl_and_offset): Likewise.
(offset_valid_for_tracked_p): New predicate.
(count_uses): Do not track locations with invalid offsets.
(add_uses): Likewise.
(add_stores): Likewise.
2007-11-07 Eric Botcazou <ebotcazou@libertysurf.fr>
* gcc.dg/out-of-bounds-1.c: New test.
--
Eric Botcazou
[-- Attachment #2: pr33822.diff --]
[-- Type: text/x-diff, Size: 5460 bytes --]
Index: rtl.h
===================================================================
--- rtl.h (revision 129844)
+++ rtl.h (working copy)
@@ -1210,8 +1210,8 @@ do { \
refer to part of a DECL. */
#define REG_EXPR(RTX) (REG_ATTRS (RTX) == 0 ? 0 : REG_ATTRS (RTX)->decl)
-/* For a MEM rtx, the offset from the start of MEM_DECL, if known, as a
- RTX that is always a CONST_INT. */
+/* For a REG rtx, the offset from the start of REG_EXPR, if known, as an
+ HOST_WIDE_INT. */
#define REG_OFFSET(RTX) (REG_ATTRS (RTX) == 0 ? 0 : REG_ATTRS (RTX)->offset)
/* Copy the attributes that apply to memory locations from RHS to LHS. */
Index: var-tracking.c
===================================================================
--- var-tracking.c (revision 129844)
+++ var-tracking.c (working copy)
@@ -267,6 +267,9 @@ typedef const struct variable_def *const
/* Pointer to the BB's information specific to variable tracking pass. */
#define VTI(BB) ((variable_tracking_info) (BB)->aux)
+/* Macro to access MEM_OFFSET as an HOST_WIDE_INT. Evaluates MEM twice. */
+#define INT_MEM_OFFSET(mem) (MEM_OFFSET (mem) ? INTVAL (MEM_OFFSET (mem)) : 0)
+
/* Alloc pool for struct attrs_def. */
static alloc_pool attrs_pool;
@@ -986,7 +989,7 @@ var_mem_set (dataflow_set *set, rtx loc,
rtx set_src)
{
tree decl = MEM_EXPR (loc);
- HOST_WIDE_INT offset = MEM_OFFSET (loc) ? INTVAL (MEM_OFFSET (loc)) : 0;
+ HOST_WIDE_INT offset = INT_MEM_OFFSET (loc);
decl = var_debug_decl (decl);
@@ -1005,7 +1008,7 @@ var_mem_delete_and_set (dataflow_set *se
enum var_init_status initialized, rtx set_src)
{
tree decl = MEM_EXPR (loc);
- HOST_WIDE_INT offset = MEM_OFFSET (loc) ? INTVAL (MEM_OFFSET (loc)) : 0;
+ HOST_WIDE_INT offset = INT_MEM_OFFSET (loc);
decl = var_debug_decl (decl);
@@ -1025,7 +1028,7 @@ static void
var_mem_delete (dataflow_set *set, rtx loc, bool clobber)
{
tree decl = MEM_EXPR (loc);
- HOST_WIDE_INT offset = MEM_OFFSET (loc) ? INTVAL (MEM_OFFSET (loc)) : 0;
+ HOST_WIDE_INT offset = INT_MEM_OFFSET (loc);
decl = var_debug_decl (decl);
if (clobber)
@@ -1642,6 +1645,18 @@ track_expr_p (tree expr)
return 1;
}
+/* Return true if OFFSET is a valid offset for a register or memory
+ access we want to track. This is used to reject out-of-bounds
+ accesses that can cause assertions to fail later. Note that we
+ don't reject negative offsets because they can be generated for
+ paradoxical subregs on big-endian architectures. */
+
+static inline bool
+offset_valid_for_tracked_p (HOST_WIDE_INT offset)
+{
+ return (-MAX_VAR_PARTS < offset) && (offset < MAX_VAR_PARTS);
+}
+
/* Determine whether a given LOC refers to the same variable part as
EXPR+OFFSET. */
@@ -1662,7 +1677,7 @@ same_variable_part_p (rtx loc, tree expr
else if (MEM_P (loc))
{
expr2 = MEM_EXPR (loc);
- offset2 = MEM_OFFSET (loc) ? INTVAL (MEM_OFFSET (loc)) : 0;
+ offset2 = INT_MEM_OFFSET (loc);
}
else
return false;
@@ -1740,7 +1755,8 @@ count_uses (rtx *loc, void *insn)
}
else if (MEM_P (*loc)
&& MEM_EXPR (*loc)
- && track_expr_p (MEM_EXPR (*loc)))
+ && track_expr_p (MEM_EXPR (*loc))
+ && offset_valid_for_tracked_p (INT_MEM_OFFSET (*loc)))
{
VTI (bb)->n_mos++;
}
@@ -1776,7 +1792,9 @@ add_uses (rtx *loc, void *insn)
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
- if (REG_EXPR (*loc) && track_expr_p (REG_EXPR (*loc)))
+ if (REG_EXPR (*loc)
+ && track_expr_p (REG_EXPR (*loc))
+ && offset_valid_for_tracked_p (REG_OFFSET (*loc)))
{
mo->type = MO_USE;
mo->u.loc = var_lowpart (mode_for_reg_attrs (*loc), *loc);
@@ -1790,7 +1808,8 @@ add_uses (rtx *loc, void *insn)
}
else if (MEM_P (*loc)
&& MEM_EXPR (*loc)
- && track_expr_p (MEM_EXPR (*loc)))
+ && track_expr_p (MEM_EXPR (*loc))
+ && offset_valid_for_tracked_p (INT_MEM_OFFSET (*loc)))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
@@ -1824,8 +1843,9 @@ add_stores (rtx loc, const_rtx expr, voi
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
if (GET_CODE (expr) == CLOBBER
- || ! REG_EXPR (loc)
- || ! track_expr_p (REG_EXPR (loc)))
+ || !(REG_EXPR (loc)
+ && track_expr_p (REG_EXPR (loc))
+ && offset_valid_for_tracked_p (REG_OFFSET (loc))))
{
mo->type = MO_CLOBBER;
mo->u.loc = loc;
@@ -1859,7 +1879,8 @@ add_stores (rtx loc, const_rtx expr, voi
}
else if (MEM_P (loc)
&& MEM_EXPR (loc)
- && track_expr_p (MEM_EXPR (loc)))
+ && track_expr_p (MEM_EXPR (loc))
+ && offset_valid_for_tracked_p (INT_MEM_OFFSET (loc)))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
@@ -1885,8 +1906,7 @@ add_stores (rtx loc, const_rtx expr, voi
{
if (same_variable_part_p (SET_SRC (expr),
MEM_EXPR (loc),
- MEM_OFFSET (loc)
- ? INTVAL (MEM_OFFSET (loc)) : 0))
+ INT_MEM_OFFSET (loc)))
mo->type = MO_COPY;
else
mo->type = MO_SET;
@@ -3075,7 +3095,7 @@ vt_get_decl_and_offset (rtx rtl, tree *d
if (MEM_ATTRS (rtl))
{
*declp = MEM_EXPR (rtl);
- *offsetp = MEM_OFFSET (rtl) ? INTVAL (MEM_OFFSET (rtl)) : 0;
+ *offsetp = INT_MEM_OFFSET (rtl);
return true;
}
}
[-- Attachment #3: out-of-bounds-1.c --]
[-- Type: text/x-csrc, Size: 390 bytes --]
/* PR rtl-optimization/33822 */
/* Origin: Andrew Pinski <pinskia@gcc.gnu.org> */
/* { dg-do compile } */
/* { dg-options "-O -g" } */
/* { dg-options "-O -g -mstrict-align" { target powerpc*-*-* } } */
void ProjectOverlay(const float localTextureAxis[2], char *lump)
{
const void *d = &localTextureAxis;
int size = sizeof(float)*8 ;
__builtin_memcpy( &lump[ 0 ], d, size );
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fix PR rtl-optimization/33822
2007-11-07 20:49 Fix PR rtl-optimization/33822 Eric Botcazou
@ 2007-11-09 22:14 ` Richard Sandiford
2007-11-12 15:39 ` Eric Botcazou
0 siblings, 1 reply; 10+ messages in thread
From: Richard Sandiford @ 2007-11-09 22:14 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Replying because I touched the same code recently...
Eric Botcazou <ebotcazou@libertysurf.fr> writes:
> +/* Return true if OFFSET is a valid offset for a register or memory
> + access we want to track. This is used to reject out-of-bounds
> + accesses that can cause assertions to fail later. Note that we
> + don't reject negative offsets because they can be generated for
> + paradoxical subregs on big-endian architectures. */
> +
> +static inline bool
> +offset_valid_for_tracked_p (HOST_WIDE_INT offset)
> +{
> + return (-MAX_VAR_PARTS < offset) && (offset < MAX_VAR_PARTS);
> +}
I don't really understand why you're allowing MAX_VAR_PARTS * 2 - 1
different offsets here though. I assume that this bug could still
occur in some situations, for other examples of invalid code?
FWIW, my 2007-10-12 mainline patch was designed to avoid the paradoxical
subreg situation you mentioned, so I think we could instead use a bound
of [0, MAX_VAR_PARTS) for mainline.
Also, do we expect to see out-of-bound offsets for REGs? Or should
we have a gcc_assert (offset_valid_for_tracked_p ...) for the REG case
instead?
Richard
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fix PR rtl-optimization/33822
2007-11-09 22:14 ` Richard Sandiford
@ 2007-11-12 15:39 ` Eric Botcazou
2007-11-15 12:31 ` Richard Sandiford
0 siblings, 1 reply; 10+ messages in thread
From: Eric Botcazou @ 2007-11-12 15:39 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> I don't really understand why you're allowing MAX_VAR_PARTS * 2 - 1
> different offsets here though. I assume that this bug could still
> occur in some situations, for other examples of invalid code?
That seemed good enough in practice to me. But you're probably right.
> FWIW, my 2007-10-12 mainline patch was designed to avoid the paradoxical
> subreg situation you mentioned, so I think we could instead use a bound
> of [0, MAX_VAR_PARTS) for mainline.
The test is exercised very early so we must let small negative offsets go
through.
> Also, do we expect to see out-of-bound offsets for REGs?
Yes, Andrew's testcase generates them on mainline.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fix PR rtl-optimization/33822
2007-11-12 15:39 ` Eric Botcazou
@ 2007-11-15 12:31 ` Richard Sandiford
2007-11-15 12:44 ` RFA: Fix tracking of pass-by-reference parameters Richard Sandiford
2007-12-16 12:44 ` Fix PR rtl-optimization/33822 Eric Botcazou
0 siblings, 2 replies; 10+ messages in thread
From: Richard Sandiford @ 2007-11-15 12:31 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Eric Botcazou <ebotcazou@libertysurf.fr> writes:
>> I don't really understand why you're allowing MAX_VAR_PARTS * 2 - 1
>> different offsets here though. I assume that this bug could still
>> occur in some situations, for other examples of invalid code?
>
> That seemed good enough in practice to me. But you're probably right.
>
>> FWIW, my 2007-10-12 mainline patch was designed to avoid the paradoxical
>> subreg situation you mentioned, so I think we could instead use a bound
>> of [0, MAX_VAR_PARTS) for mainline.
>
> The test is exercised very early so we must let small negative offsets go
> through.
But it's easy to refactor the code slightly so that it checks the
adjusted offset; see track_loc_p in the patch below.
>> Also, do we expect to see out-of-bound offsets for REGs?
>
> Yes, Andrew's testcase generates them on mainline.
Hmm, OK. I was still curious whether we should see them in valid code,
especially negative offsets. And what a can of worms that turned out
to be. There are lots of bugs here...
First off, how is REG_OFFSET supposed to be calculated? Is it calculated
in the same way as SUBREG_BYTE, with paradoxical lowparts having an offset
of zero? Or is the REG_OFFSET for (reg:M R) simply the offset of the
first byte in the M-mode value? We aren't consistent at the moment.
E.g. take the following insn from compile/980726-1.c, compiled at -O2 -g
for mipsisa64-elf:
(insn 75 30 93 3 .../980726-1.c:4 (set (reg:DI 5 $5 [orig:200 i ] [200])
(ashift:DI (reg:DI 6 $6 [orig:199 i+-4 ] [199])
(const_int 32 [0x20]))) 326 {*ashldi3} (nil))
Here we have two 64-bit accesses for a 32-bit value "i". One of the
accesses uses an offset of 0 (as for SUBREG_BYTE) while the other uses
an offset of -4 (offset of the first byte in the DImode value). This
leads to wrong debug information for this function, because we don't
consider the assignment to (reg:DI 5) to be a full assignment to "i"; we
assume that the stuff at "-4" in (reg:DI 6) is still valid. There are
several other examples of this.
The REG_OFFSET question is related to PR 14084:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=14084
and the patch for that bug went the SUBREG_BYTE route:
tree decl;
HOST_WIDE_INT var_size;
/* PR middle-end/14084
The problem appears when a variable is stored in a larger register
and later it is used in the original mode or some mode in between
or some part of variable is accessed.
On little endian machines there is no problem because
the REG_OFFSET of the start of the variable is the same when
accessed in any mode (it is 0).
However, this is not true on big endian machines.
The offset of the start of the variable is different when accessed
in different modes.
When we are taking a part of the REG we have to change the OFFSET
from offset WRT size of mode of REG to offset WRT size of variable.
If we would not do the big endian correction the resulting REG_OFFSET
would be larger than the size of the DECL.
Examples of correction, for BYTES_BIG_ENDIAN WORDS_BIG_ENDIAN machine:
REG.mode MODE DECL size old offset new offset description
DI SI 4 4 0 int32 in SImode
DI SI 1 4 0 char in SImode
DI QI 1 7 0 char in QImode
DI QI 4 5 1 1st element in QImode
of char[4]
DI HI 4 6 2 1st element in HImode
of int16[2]
If the size of DECL is equal or greater than the size of REG
we can't do this correction because the register holds the
whole variable or a part of the variable and thus the REG_OFFSET
is already correct. */
decl = REG_EXPR (reg);
if ((BYTES_BIG_ENDIAN || WORDS_BIG_ENDIAN)
&& decl != NULL
&& offset > 0
&& GET_MODE_SIZE (GET_MODE (reg)) > GET_MODE_SIZE (GET_MODE (new))
&& ((var_size = int_size_in_bytes (TREE_TYPE (decl))) > 0
&& var_size < GET_MODE_SIZE (GET_MODE (reg))))
{
int offset_le;
/* Convert machine endian to little endian WRT size of mode of REG. */
if (WORDS_BIG_ENDIAN)
offset_le = ((GET_MODE_SIZE (GET_MODE (reg)) - 1 - offset)
/ UNITS_PER_WORD) * UNITS_PER_WORD;
else
offset_le = (offset / UNITS_PER_WORD) * UNITS_PER_WORD;
if (BYTES_BIG_ENDIAN)
offset_le += ((GET_MODE_SIZE (GET_MODE (reg)) - 1 - offset)
% UNITS_PER_WORD);
else
offset_le += offset % UNITS_PER_WORD;
if (offset_le >= var_size)
{
/* MODE is wider than the variable so the new reg will cover
the whole variable so the resulting OFFSET should be 0. */
offset = 0;
}
else
{
/* Convert little endian to machine endian WRT size of variable. */
if (WORDS_BIG_ENDIAN)
offset = ((var_size - 1 - offset_le)
/ UNITS_PER_WORD) * UNITS_PER_WORD;
else
offset = (offset_le / UNITS_PER_WORD) * UNITS_PER_WORD;
if (BYTES_BIG_ENDIAN)
offset += ((var_size - 1 - offset_le)
% UNITS_PER_WORD);
else
offset += offset_le % UNITS_PER_WORD;
}
}
This code copes with the case where we're narrowing a register that is
wider than the decl. For example, it copes with cases where we have
a 32-bit lowpart subreg of a 64-bit REG, and where that 64-bit REG
is associated with a 32-bit decl. Both the old 64-bit REG and the
new 32-bit REG will then have offsets of 0.
However, the code doesn't cope with other cases, such as when we're
taking the paradoxical lowpart of a register. On a big-endian 64-bit
target, if we have a 32-bit REG associated with a 32-bit decl, and if
we then take the paradoxical 64-bit lowpart of that REG, the incoming
offset will be -4. (And it's intentional that the incoming offset is
-4; it's the correct value when refering to a 64-bit decl "i", because
the 32-bit REG would then be associated with "i+4".)
The offset for a promoted REG parameter is always zero, which can
be another source of a (reg:M R [i]) / (reg:M R [i+-x]) discrepancy.
An example of this is in gcc.c-torture/compile/20020415-1.c
for mipsisa64-elf, compiled with -O2 -g. The instruction stream
uses (reg:DI $4 [x+-4]), but the parameter's rtl is (reg:DI $4 [x]).
We then don't properly treat "x" as unavailable when assigning
an untrackable value to $4.
So, going back to the original REG_OFFSET question, I think it would make
more sense for the REG_OFFSET of (reg:M R) to be the offset of the first
byte in the M-mode value. That's what MEM_OFFSET is -- more later about
why we need negative offsets there too -- and var-tracking.c fundamentally
assumes that memory offsets and register offsets are comparable.
Luckily, the code that sets REG_OFFSET is localised to emit-rtl.c, with
the rest of the compiler using higher-level interfaces. (Thanks Jan!)
So it's easy to enforce a consistent representation. Also, the problem
identified in PR 14084 resurfaces if you revert the associated patch,
so it's easy to verify that this alternative approach fixes it.
So, the patch below:
- Removes the 14084 code.
- Adds a byte_lowpart_offset function to go alongside
subreg_lowpart_offset. Like subreg_lowpart_offset, this new function
copes with identity lowparts, true lowparts and paradoxical lowparts.
The offset can be negative for paradoxical lowparts on big-endian
systems.
- Makes sure that an M-mode REG for an N-mode decl starts off
with a REG_OFFSET of "byte_lowpart (M, N)".
That fixes the worst source of wrong offsets (and thus wrong debug
information in the var-tracking output). However, there are a few
other cases:
- regclass.c:reg_scan_mark_refs has code to copy register attributes
from the source of a set to the destination, including cases where
the source is a truncation, subreg or extension. I'm going to take
it on faith that this sort of copying is a good idea and that this
is (for now) the place to do it. I assume that we'll do this better
after we merge one of the debug branches.
However, the code doesn't account for offset changes induced by the
truncation, subreg or extension:
/* If this is setting a register from a register or from a simple
conversion of a register, propagate REG_EXPR. */
if (REG_P (dest))
{
rtx src = SET_SRC (x);
while (GET_CODE (src) == SIGN_EXTEND
|| GET_CODE (src) == ZERO_EXTEND
|| GET_CODE (src) == TRUNCATE
|| (GET_CODE (src) == SUBREG && subreg_lowpart_p (src)))
src = XEXP (src, 0);
if (!REG_ATTRS (dest) && REG_P (src))
REG_ATTRS (dest) = REG_ATTRS (src);
if (!REG_ATTRS (dest) && MEM_P (src))
set_reg_attrs_from_mem (dest, src);
}
This is wrong for big-endian targets regardless of the REG_OFFSET
question above. E.g., suppose "i" is a 64-bit decl and we have
the following code on a 32-bit big-endian target:
(set (reg:DI R1) (sign_extend:SI (reg:SI R2 [i+4])))
We must associate R1 with "i" rather than "i+4". Also,
on 64-bit big-endian MIPS targets, if we have:
(set (reg:SI R1) (truncate:SI (reg:DI R2 [i])))
we must associate R1 with "i+4" rather than "i".
gcc.c-torture/compile/920413-1.c is an example of the latter
problem, when compiled with -O2 -g on mipsisa64-elf. Again,
it leads to wrong location info.
The patch replaces set_reg_attrs_from_mem with a more general
(but still simple) set_reg_attrs_from_value. The new function
can copy attributes from REGs as well as MEMs and takes the offset
adjustment into account.
- combine.c has code to change the mode of pseudo registers in-place.
This code does not consider the effect of the mode change on the
REG_OFFSET. Again, this is wrong regardless of the REG_OFFSET
representation, because it affects non-paradoxical cases too.
And it again leads to wrong debug info in the var-tracking output.
The patch adds a new adjust_reg_mode function that adjusts
both the mode and the REG_OFFSET.
- reload similarly changes the mode and address of a MEM in a
SUBREG if the MEM has an eliminated address that needs reloading:
/* If the address changes because of register elimination, then
it must be replaced. */
if (force_replace
|| ! rtx_equal_p (tem, reg_equiv_mem[regno]))
{
[...]
XEXP (tem, 0) = plus_constant (XEXP (tem, 0), offset);
PUT_MODE (tem, GET_MODE (x));
It doesn't change the MEM_OFFSET though. Again, this is wrong
as things stand.
The patch calls set_mem_offset in this case.
And that's the end of this particular story as far as recording
the offsets goes. There are also some problems in the way
var-tracking uses them:
- My original patch for paradoxical lowparts wasn't aggressive enough.
We also need to cope with promoted variables, such as 32-bit variables
stored in 64-bit pseudo registers.
- We need to do the same when setting up the locations of
promoted parameters.
- We need to do the same when a 64-bit promoted variable is spilled
to the stack. We currently emit a memory location for the start
of the spill slot, which is wrong on big-endian targets when the
slot is padded.
- We currently assume that a store to a register that holds VAR+OFFSET
does not clobber [VAR+0, VAR+OFFSET). That's wrong when the register
is big enough to hold the whole of VAR. E.g. it's perfectly possible
for a target to store to (reg:SI R) and then reference the value using
(reg:DI R), where (reg:DI R) is a single register.
Again, these are all problems that lead to wrong debug info. And luckily,
they're not really separate problems at all. The idea is that if we see
a reference to VAR with an offset consistent with a lowpart of VAR and
either:
1) that lowpart is paradoxical; or
2) we're storing into a register that can hold the whole of VAR
then we should consider this to be an access to the whole of VAR.
After those changes, we can enforce the [0, MAX_VAR_PARTS) range,
thus avoiding the problem that sparked my original reply.
(Rejecting negative offsets has other benefits too; see later.)
I found one further problem: if the ABI requires something to be
passed by invisible reference, we attach attributes to both the
pointer and the dereferenced value. This leads to awful confusion.
E.g. if a complex is passed by reference (as the MIPS EABI sometimes
requires) we can mistake the pointer to that complex for either the
real or imaginary part. I'll post a follow-on patch for that.
Now I realise that the patch might be a bit daunting, but I think it's
actually very localised, and it's all bug fixes. I really do think it's
stage 3 material. However, because it isn't exactly a one-liner either,
I've tried to go an extra mile as far as testing goes.
First, I did the normal bootstrap & regression test on x86_64-linux-gnu,
and a regression test on mipsisa64-elf (using my usual test flags of
{-EB,-EL}{,-msoft-float}). I then retested mipsisa64-elf with:
if (!offset_valid_for_tracked_p (offset))
return false;
replaced by:
if (!offset_valid_for_tracked_p (offset))
gcc_unreachable ();
So, to answer the question "do we expect to see negative corrected
offsets in valid code?": there is one semi-legitimate case. If we sign-
or zero-extend a value, the regclass.c code I quoted above will associate
the destination of the extension with the source. Thus if the extended
value occupies two words, and if the extension is later split into
word-mode operations, the high word will end up with a negative offset
on big-endian targets. We should disregard that high part, as it lies
completely outside the variable. So returning false here is the right
thing to do.
So, I tried the mipsisa64-elf testing again with:
if (!offset_valid_for_tracked_p (offset))
{
gcc_assert (offset <= -UNITS_PER_WORD);
return false;
}
instead. It built fine, and the only regressions in the C, C++,
objc and libstdc++ testsuites were -- as expected -- in the test
you added for this PR:
FAIL: gcc.dg/out-of-bounds-1.c (internal compiler error)
FAIL: gcc.dg/out-of-bounds-1.c (test for excess errors)
This gives me at least some confidence that big-endian offsets are
much more accurate than they used to be.
Second, I compared the GDB test results before and after the patch
below (i.e. without the follow-up) on mipsisa64-elf, using the flags
{,-fvar-tracking}{-mips64,-mips32}. As expected, the results were
identical without -fvar-tracking. The patch fixes two cases
for "-mips64 -fvar-tracking":
FAIL: gdb.base/store.exp: var charest l; print incremented l, expecting 2 ..002.
FAIL: gdb.base/store.exp: var short l; print incremented l, expecting 2
and introduces no regressions. For "-mips32 -fvar-tracking" it fixes:
FAIL: gdb.base/call-ar-st.exp: continue to 1300 (the program exited)
FAIL: gdb.base/call-ar-st.exp: step into init_bit_flags_combo
FAIL: gdb.base/call-ar-st.exp: print print_bit_flags_combo from init_bit_flags_combo
FAIL: gdb.base/call-ar-st.exp: continue to 1305 (the program is no longer running)
FAIL: gdb.base/call-ar-st.exp: continue to 1311 (the program is no longer running)
FAIL: gdb.base/call-ar-st.exp: print sum_struct_print(10, *struct1, *struct2, *struct3, *struct4)
FAIL: gdb.base/call-ar-st.exp: print print_struct_rep(*struct1, *struct2, *struct3) (pattern 1)
FAIL: gdb.base/call-ar-st.exp: print print_one_large_struct(*list1)
FAIL: gdb.base/funcargs.exp: continue to call5b (sizeof long == sizeof int)
FAIL: gdb.base/funcargs.exp: print un (sizeof long == sizeof int)
FAIL: gdb.base/store.exp: var charest l; print incremented l, expecting 2 ..002.
FAIL: gdb.base/store.exp: var short l; print incremented l, expecting 2
and turns these failures:
FAIL: gdb.base/call-ar-st.exp: print print_small_structs from print_long_arg_list (pattern 1)
FAIL: gdb.base/call-ar-st.exp: print print_long_arg_list (pattern 1)
into these later failures:
FAIL: gdb.base/call-ar-st.exp: print print_small_structs from print_long_arg_list (pattern 6)
FAIL: gdb.base/call-ar-st.exp: print print_long_arg_list (pattern 26)
In other words, we now have 30 matching patterns where previously we
failed straight away.
I then ran the GDB testsuite with both the patch below and the follow-up
applied. The results were no different from those with just the patch
below applied.
To make sure there were no ill effects on a little-endian target,
I ran the gdb testsuite on x86_64-linux-gnu before and after the
combined patch, both with default options and with -fvar-tracking.
As expected, the results were identical.
Finally, I compiled the testsuite before and after this patch (without
the follow-on) with "-O2 -g" on mipsisa64-elf and compared the
differences. I filtered out:
- cases where the before and after code was identical modulo label
renumbering,
- cases where we got the wrong offset for a 32-bit value in a 64-bit
stack slot,
- cases where (due to consistent offset numbering) we were able to
combine ranges of the form "location L1 for [R1, R2); location L1
for [R2, R3)" into "location L1 for [R1, R3)"
There were too many changes to go through by hand, so I just looked
at the ones in gcc.c-torture/compile. They divide up as follows:
- gcc.c-torture/compile/20001121-1.c
- gcc.c-torture/compile/920413-1.c
- gcc.c-torture/compile/20001121-1.c
- gcc.c-torture/compile/920909-1.c
- gcc.c-torture/compile/930120-1.c
- gcc.c-torture/compile/compound-literal-3.c
- gcc.c-torture/compile/pr22589-1.c
- gcc.c-torture/compile/complex-3.c
Cases where a single-register value is assigned via a nonparadoxical
lowpart, which we now recognise as a full definition. This includes
cases where we had a truncation with the wrong REG_OFFSET, as
explained above.
- gcc.c-torture/compile/20020415-1.c
- gcc.c-torture/compile/20070501-1.c
- gcc.c-torture/compile/980726-1.c
- gcc.c-torture/compile/pr24227.c
- gcc.c-torture/compile/pr28776-1.c
- gcc.c-torture/compile/pr29201.c
Cases where we had +0 and -4 DImode references to the same
variable, and in which we therefore failed to recognise
that an assignment at +0 was a full definition.
- gcc.c-torture/compile/920928-2.c
Likewise, but with +0 and -2 SImode references to the same
16-bit variable.
- gcc.c-torture/compile/20050206-1.c
The patched compiler adds an empty location list in cases where
we previously had no location at all. This can happen before and
after the patch (e.g. in gcc.dg/tree-ssa/flatten-1.c, amongst others).
- gcc.c-torture/compile/pr32349.c
The patched compiler adds empty ranges to a location list.
Again, you can get empty ranges before and after the patch
(e.g. in gcc.c-torture/compile/920501-15.c, amongst others).
- gcc.c-torture/compile/920710-2.c
- gcc.c-torture/compile/pr24883.c
The unpatched compiler generated location lists that span the
whole function and that specify the same location for each
range in the list. The patched compiler now generates a
single location (not a list).
- gcc.c-torture/compile/941019-1.c
- gcc.c-torture/compile/vector-2.c
Cases where both the patched and unpatched compilers are confused
about pass-by-reference parameters. The follow-on patch fixes both
testcases.
OK to install?
Richard
gcc/
* rtl.h (set_reg_attrs_from_mem): Rename to...
(set_reg_attrs_from_value): ...this.
(adjust_reg_mode, byte_lowpart_offset): Declare.
* emit-rtl.c (byte_lowpart_offset): New function.
(update_reg_offset): Remove special offset handling for big-endian
targets.
(adjust_reg_mode): New function.
(set_reg_attrs_for_mem): Rename to...
(set_reg_attrs_for_value): ...this and generalize to all values.
If the register is a lowpart of the value, adjust the offset
accordingly.
(set_reg_attrs_for_parm): Update after the above renaming.
(set_reg_attrs_for_decl_rtl): New function, split out from
set_decl_incoming_rtl. Set the offset of plain REGs to the
offset of the REG's mode from the decl's. Assert that all
subregs are lowparts and handle their inner registers in the
same way as plain REGs.
(set_decl_rtl, set_incoming_decl_rtl): Use reg_attrs_for_decl_rtl.
* combine.c (do_SUBST_MODE, try_combine, undo_all): Use adjust_reg_mode
instead of PUT_MODE.
* regclass.c (reg_scan_mark_refs): Use set_reg_attrs_from_value.
* reload.c (find_reloads_subreg_address): Call set_mem_offset
when offseting a MEM.
* var-tracking.c (offset_valid_for_tracked_p): Only allow offsets
in the range [0, MAX_VAR_PARTS).
(mode_for_reg_attrs): Replace with...
(track_loc_p): ...this new function. Return the mode and offset
to the caller, checking that the latter is valid. If the rtx is
a paradoxical lowpart of the decl, use the decl's mode instead.
Do the same when storing to a register that contains the entire decl.
(var_lowpart): Use byte_lowpart_offset rather than
subreg_lowpart_offset when adjusting the offset attribute.
(count_uses, add_uses, add_stores): Use track_reg_p instead of
REG_EXPR, MEM_EXPR, REG_OFFSET, INT_MEM_OFFSET, track_expr_p,
offset_valid_for_tracked_p and mode_for_reg_attrs. Generate
lowparts for MEMs as well as REGs.
(vt_add_function_parameters): When obtaining the information from
the decl_rtl, adjust the offset to match incoming. Use track_loc_p
and var_lowpart.
Index: gcc/rtl.h
===================================================================
*** gcc/rtl.h 2007-11-14 10:54:36.000000000 +0000
--- gcc/rtl.h 2007-11-14 10:54:36.000000000 +0000
*************** extern rtx copy_insn_1 (rtx);
*** 1476,1484 ****
extern rtx copy_insn (rtx);
extern rtx gen_int_mode (HOST_WIDE_INT, enum machine_mode);
extern rtx emit_copy_of_insn_after (rtx, rtx);
! extern void set_reg_attrs_from_mem (rtx, rtx);
extern void set_mem_attrs_from_reg (rtx, rtx);
extern void set_reg_attrs_for_parm (rtx, rtx);
extern int mem_expr_equal_p (const_tree, const_tree);
/* In rtl.c */
--- 1476,1485 ----
extern rtx copy_insn (rtx);
extern rtx gen_int_mode (HOST_WIDE_INT, enum machine_mode);
extern rtx emit_copy_of_insn_after (rtx, rtx);
! extern void set_reg_attrs_from_value (rtx, rtx);
extern void set_mem_attrs_from_reg (rtx, rtx);
extern void set_reg_attrs_for_parm (rtx, rtx);
+ extern void adjust_reg_mode (rtx, enum machine_mode);
extern int mem_expr_equal_p (const_tree, const_tree);
/* In rtl.c */
*************** extern unsigned int subreg_lowpart_offse
*** 1522,1527 ****
--- 1523,1529 ----
enum machine_mode);
extern unsigned int subreg_highpart_offset (enum machine_mode,
enum machine_mode);
+ extern int byte_lowpart_offset (enum machine_mode, enum machine_mode);
extern rtx make_safe_from (rtx, rtx);
extern rtx convert_memory_address (enum machine_mode, rtx);
extern rtx get_insns (void);
Index: gcc/emit-rtl.c
===================================================================
*** gcc/emit-rtl.c 2007-11-14 19:03:52.000000000 +0000
--- gcc/emit-rtl.c 2007-11-15 09:54:32.000000000 +0000
*************** gen_rtvec_v (int n, rtx *argp)
*** 837,842 ****
--- 837,857 ----
return rt_val;
}
\f
+ /* Return the byte offset of an OUTER_MODE value from an INNER_MODE value,
+ given that the former is a lowpart of the latter. It may be a paradoxical
+ lowpart, in which case the offset might be negative on big-endian
+ targets. */
+
+ int
+ byte_lowpart_offset (enum machine_mode outer_mode,
+ enum machine_mode inner_mode)
+ {
+ if (GET_MODE_SIZE (outer_mode) < GET_MODE_SIZE (inner_mode))
+ return subreg_lowpart_offset (outer_mode, inner_mode);
+ else
+ return -subreg_lowpart_offset (inner_mode, outer_mode);
+ }
+ \f
/* Generate a REG rtx for a new pseudo register of mode MODE.
This pseudo is assigned the next sequential register number. */
*************** gen_reg_rtx (enum machine_mode mode)
*** 897,985 ****
static void
update_reg_offset (rtx new, rtx reg, int offset)
{
- tree decl;
- HOST_WIDE_INT var_size;
-
- /* PR middle-end/14084
- The problem appears when a variable is stored in a larger register
- and later it is used in the original mode or some mode in between
- or some part of variable is accessed.
-
- On little endian machines there is no problem because
- the REG_OFFSET of the start of the variable is the same when
- accessed in any mode (it is 0).
-
- However, this is not true on big endian machines.
- The offset of the start of the variable is different when accessed
- in different modes.
- When we are taking a part of the REG we have to change the OFFSET
- from offset WRT size of mode of REG to offset WRT size of variable.
-
- If we would not do the big endian correction the resulting REG_OFFSET
- would be larger than the size of the DECL.
-
- Examples of correction, for BYTES_BIG_ENDIAN WORDS_BIG_ENDIAN machine:
-
- REG.mode MODE DECL size old offset new offset description
- DI SI 4 4 0 int32 in SImode
- DI SI 1 4 0 char in SImode
- DI QI 1 7 0 char in QImode
- DI QI 4 5 1 1st element in QImode
- of char[4]
- DI HI 4 6 2 1st element in HImode
- of int16[2]
-
- If the size of DECL is equal or greater than the size of REG
- we can't do this correction because the register holds the
- whole variable or a part of the variable and thus the REG_OFFSET
- is already correct. */
-
- decl = REG_EXPR (reg);
- if ((BYTES_BIG_ENDIAN || WORDS_BIG_ENDIAN)
- && decl != NULL
- && offset > 0
- && GET_MODE_SIZE (GET_MODE (reg)) > GET_MODE_SIZE (GET_MODE (new))
- && ((var_size = int_size_in_bytes (TREE_TYPE (decl))) > 0
- && var_size < GET_MODE_SIZE (GET_MODE (reg))))
- {
- int offset_le;
-
- /* Convert machine endian to little endian WRT size of mode of REG. */
- if (WORDS_BIG_ENDIAN)
- offset_le = ((GET_MODE_SIZE (GET_MODE (reg)) - 1 - offset)
- / UNITS_PER_WORD) * UNITS_PER_WORD;
- else
- offset_le = (offset / UNITS_PER_WORD) * UNITS_PER_WORD;
-
- if (BYTES_BIG_ENDIAN)
- offset_le += ((GET_MODE_SIZE (GET_MODE (reg)) - 1 - offset)
- % UNITS_PER_WORD);
- else
- offset_le += offset % UNITS_PER_WORD;
-
- if (offset_le >= var_size)
- {
- /* MODE is wider than the variable so the new reg will cover
- the whole variable so the resulting OFFSET should be 0. */
- offset = 0;
- }
- else
- {
- /* Convert little endian to machine endian WRT size of variable. */
- if (WORDS_BIG_ENDIAN)
- offset = ((var_size - 1 - offset_le)
- / UNITS_PER_WORD) * UNITS_PER_WORD;
- else
- offset = (offset_le / UNITS_PER_WORD) * UNITS_PER_WORD;
-
- if (BYTES_BIG_ENDIAN)
- offset += ((var_size - 1 - offset_le)
- % UNITS_PER_WORD);
- else
- offset += offset_le % UNITS_PER_WORD;
- }
- }
-
REG_ATTRS (new) = get_reg_attrs (REG_EXPR (reg),
REG_OFFSET (reg) + offset);
}
--- 912,917 ----
*************** gen_reg_rtx_offset (rtx reg, enum machin
*** 1009,1022 ****
return new;
}
! /* Set REG to the decl that MEM refers to. */
void
! set_reg_attrs_from_mem (rtx reg, rtx mem)
{
! if (MEM_OFFSET (mem) && GET_CODE (MEM_OFFSET (mem)) == CONST_INT)
REG_ATTRS (reg)
! = get_reg_attrs (MEM_EXPR (mem), INTVAL (MEM_OFFSET (mem)));
}
/* Set the register attributes for registers contained in PARM_RTX.
--- 941,970 ----
return new;
}
! /* Adjust REG in-place so that it has mode MODE. It is assumed that the
! new register is a (possibly paradoxical) lowpart of the old one. */
!
! void
! adjust_reg_mode (rtx reg, enum machine_mode mode)
! {
! update_reg_offset (reg, reg, byte_lowpart_offset (mode, GET_MODE (reg)));
! PUT_MODE (reg, mode);
! }
!
! /* Copy REG's attributes from X, if X has any attributes. If REG and X
! have different modes, REG is a (possibly paradoxical) lowpart of X. */
void
! set_reg_attrs_from_value (rtx reg, rtx x)
{
! int offset;
!
! offset = byte_lowpart_offset (GET_MODE (reg), GET_MODE (x));
! if (MEM_P (x) && MEM_OFFSET (x) && GET_CODE (MEM_OFFSET (x)) == CONST_INT)
REG_ATTRS (reg)
! = get_reg_attrs (MEM_EXPR (x), INTVAL (MEM_OFFSET (x)) + offset);
! if (REG_P (x) && REG_ATTRS (x))
! update_reg_offset (reg, x, offset);
}
/* Set the register attributes for registers contained in PARM_RTX.
*************** set_reg_attrs_from_mem (rtx reg, rtx mem
*** 1026,1032 ****
set_reg_attrs_for_parm (rtx parm_rtx, rtx mem)
{
if (REG_P (parm_rtx))
! set_reg_attrs_from_mem (parm_rtx, mem);
else if (GET_CODE (parm_rtx) == PARALLEL)
{
/* Check for a NULL entry in the first slot, used to indicate that the
--- 974,980 ----
set_reg_attrs_for_parm (rtx parm_rtx, rtx mem)
{
if (REG_P (parm_rtx))
! set_reg_attrs_from_value (parm_rtx, mem);
else if (GET_CODE (parm_rtx) == PARALLEL)
{
/* Check for a NULL entry in the first slot, used to indicate that the
*************** set_reg_attrs_for_parm (rtx parm_rtx, rt
*** 1043,1096 ****
}
}
! /* Assign the RTX X to declaration T. */
! void
! set_decl_rtl (tree t, rtx x)
! {
! DECL_WRTL_CHECK (t)->decl_with_rtl.rtl = x;
! if (!x)
! return;
! /* For register, we maintain the reverse information too. */
! if (REG_P (x))
! REG_ATTRS (x) = get_reg_attrs (t, 0);
! else if (GET_CODE (x) == SUBREG)
! REG_ATTRS (SUBREG_REG (x))
! = get_reg_attrs (t, -SUBREG_BYTE (x));
! if (GET_CODE (x) == CONCAT)
! {
! if (REG_P (XEXP (x, 0)))
! REG_ATTRS (XEXP (x, 0)) = get_reg_attrs (t, 0);
! if (REG_P (XEXP (x, 1)))
! REG_ATTRS (XEXP (x, 1))
! = get_reg_attrs (t, GET_MODE_UNIT_SIZE (GET_MODE (XEXP (x, 0))));
! }
! if (GET_CODE (x) == PARALLEL)
{
! int i;
! for (i = 0; i < XVECLEN (x, 0); i++)
! {
! rtx y = XVECEXP (x, 0, i);
! if (REG_P (XEXP (y, 0)))
! REG_ATTRS (XEXP (y, 0)) = get_reg_attrs (t, INTVAL (XEXP (y, 1)));
! }
}
- }
-
- /* Assign the RTX X to parameter declaration T. */
- void
- set_decl_incoming_rtl (tree t, rtx x)
- {
- DECL_INCOMING_RTL (t) = x;
-
- if (!x)
- return;
- /* For register, we maintain the reverse information too. */
if (REG_P (x))
! REG_ATTRS (x) = get_reg_attrs (t, 0);
! else if (GET_CODE (x) == SUBREG)
! REG_ATTRS (SUBREG_REG (x))
! = get_reg_attrs (t, -SUBREG_BYTE (x));
if (GET_CODE (x) == CONCAT)
{
if (REG_P (XEXP (x, 0)))
--- 991,1011 ----
}
}
! /* Set the REG_ATTRS for registers in value X, given that X represents
! decl T. */
! static void
! set_reg_attrs_for_decl_rtl (tree t, rtx x)
! {
! if (GET_CODE (x) == SUBREG)
{
! gcc_assert (subreg_lowpart_p (x));
! x = SUBREG_REG (x);
}
if (REG_P (x))
! REG_ATTRS (x)
! = get_reg_attrs (t, byte_lowpart_offset (GET_MODE (x),
! TYPE_MODE (TREE_TYPE (t))));
if (GET_CODE (x) == CONCAT)
{
if (REG_P (XEXP (x, 0)))
*************** set_decl_incoming_rtl (tree t, rtx x)
*** 1119,1124 ****
--- 1034,1059 ----
}
}
+ /* Assign the RTX X to declaration T. */
+
+ void
+ set_decl_rtl (tree t, rtx x)
+ {
+ DECL_WRTL_CHECK (t)->decl_with_rtl.rtl = x;
+ if (x)
+ set_reg_attrs_for_decl_rtl (t, x);
+ }
+
+ /* Assign the RTX X to parameter declaration T. */
+
+ void
+ set_decl_incoming_rtl (tree t, rtx x)
+ {
+ DECL_INCOMING_RTL (t) = x;
+ if (x)
+ set_reg_attrs_for_decl_rtl (t, x);
+ }
+
/* Identify REG (which may be a CONCAT) as a user register. */
void
Index: gcc/combine.c
===================================================================
*** gcc/combine.c 2007-11-14 19:03:52.000000000 +0000
--- gcc/combine.c 2007-11-14 19:30:34.000000000 +0000
*************** do_SUBST_MODE (rtx *into, enum machine_m
*** 751,757 ****
buf->kind = UNDO_MODE;
buf->where.r = into;
buf->old_contents.m = oldval;
! PUT_MODE (*into, newval);
buf->next = undobuf.undos, undobuf.undos = buf;
}
--- 751,757 ----
buf->kind = UNDO_MODE;
buf->where.r = into;
buf->old_contents.m = oldval;
! adjust_reg_mode (*into, newval);
buf->next = undobuf.undos, undobuf.undos = buf;
}
*************** try_combine (rtx i3, rtx i2, rtx i1, int
*** 2984,2990 ****
{
struct undo *buf;
! PUT_MODE (regno_reg_rtx[REGNO (i2dest)], old_mode);
buf = undobuf.undos;
undobuf.undos = buf->next;
buf->next = undobuf.frees;
--- 2984,2990 ----
{
struct undo *buf;
! adjust_reg_mode (regno_reg_rtx[REGNO (i2dest)], old_mode);
buf = undobuf.undos;
undobuf.undos = buf->next;
buf->next = undobuf.frees;
*************** undo_all (void)
*** 3826,3832 ****
*undo->where.i = undo->old_contents.i;
break;
case UNDO_MODE:
! PUT_MODE (*undo->where.r, undo->old_contents.m);
break;
default:
gcc_unreachable ();
--- 3826,3832 ----
*undo->where.i = undo->old_contents.i;
break;
case UNDO_MODE:
! adjust_reg_mode (*undo->where.r, undo->old_contents.m);
break;
default:
gcc_unreachable ();
Index: gcc/regclass.c
===================================================================
*** gcc/regclass.c 2007-11-14 19:03:52.000000000 +0000
--- gcc/regclass.c 2007-11-14 19:30:34.000000000 +0000
*************** reg_scan_mark_refs (rtx x, rtx insn)
*** 2435,2444 ****
|| (GET_CODE (src) == SUBREG && subreg_lowpart_p (src)))
src = XEXP (src, 0);
! if (REG_P (src))
! REG_ATTRS (dest) = REG_ATTRS (src);
! if (MEM_P (src))
! set_reg_attrs_from_mem (dest, src);
}
/* ... fall through ... */
--- 2435,2441 ----
|| (GET_CODE (src) == SUBREG && subreg_lowpart_p (src)))
src = XEXP (src, 0);
! set_reg_attrs_from_value (dest, src);
}
/* ... fall through ... */
Index: gcc/reload.c
===================================================================
*** gcc/reload.c 2007-11-14 19:03:52.000000000 +0000
--- gcc/reload.c 2007-11-14 19:30:34.000000000 +0000
*************** find_reloads_subreg_address (rtx x, int
*** 6023,6028 ****
--- 6023,6030 ----
XEXP (tem, 0) = plus_constant (XEXP (tem, 0), offset);
PUT_MODE (tem, GET_MODE (x));
+ if (MEM_ATTRS (tem))
+ set_mem_offset (tem, MEM_OFFSET (tem) + offset);
/* If this was a paradoxical subreg that we replaced, the
resulting memory must be sufficiently aligned to allow
Index: gcc/var-tracking.c
===================================================================
*** gcc/var-tracking.c 2007-11-14 19:03:52.000000000 +0000
--- gcc/var-tracking.c 2007-11-15 10:02:40.000000000 +0000
*************** track_expr_p (tree expr)
*** 1647,1660 ****
/* Return true if OFFSET is a valid offset for a register or memory
access we want to track. This is used to reject out-of-bounds
! accesses that can cause assertions to fail later. Note that we
! don't reject negative offsets because they can be generated for
! paradoxical subregs on big-endian architectures. */
static inline bool
offset_valid_for_tracked_p (HOST_WIDE_INT offset)
{
! return (-MAX_VAR_PARTS < offset) && (offset < MAX_VAR_PARTS);
}
/* Determine whether a given LOC refers to the same variable part as
--- 1647,1658 ----
/* Return true if OFFSET is a valid offset for a register or memory
access we want to track. This is used to reject out-of-bounds
! accesses that can cause assertions to fail later. */
static inline bool
offset_valid_for_tracked_p (HOST_WIDE_INT offset)
{
! return (offset >= 0) && (offset < MAX_VAR_PARTS);
}
/* Determine whether a given LOC refers to the same variable part as
*************** same_variable_part_p (rtx loc, tree expr
*** 1691,1718 ****
return (expr == expr2 && offset == offset2);
}
! /* REG is a register we want to track. If not all of REG contains useful
! information, return the mode of the lowpart that does contain useful
! information, otherwise return the mode of REG.
! If REG was a paradoxical subreg, its REG_ATTRS will describe the
! whole subreg, but only the old inner part is really relevant. */
!
! static enum machine_mode
! mode_for_reg_attrs (rtx reg)
{
enum machine_mode mode;
! mode = GET_MODE (reg);
! if (!HARD_REGISTER_NUM_P (ORIGINAL_REGNO (reg)))
{
enum machine_mode pseudo_mode;
! pseudo_mode = PSEUDO_REGNO_MODE (ORIGINAL_REGNO (reg));
if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (pseudo_mode))
! mode = pseudo_mode;
}
! return mode;
}
/* Return the MODE lowpart of LOC, or null if LOC is not something we
--- 1689,1753 ----
return (expr == expr2 && offset == offset2);
}
! /* LOC is a REG or MEM that we would like to track if possible.
! If EXPR is null, we don't know what expression LOC refers to,
! otherwise it refers to EXPR + OFFSET. STORE_REG_P is true if
! LOC is an lvalue register.
!
! Return true if EXPR is nonnull and if LOC, or some lowpart of it,
! is something we can track. When returning true, store the mode of
! the lowpart we can track in *MODE_OUT (if nonnull) and its offset
! from EXPR in *OFFSET_OUT (if nonnull). */
! static bool
! track_loc_p (rtx loc, tree expr, HOST_WIDE_INT offset, bool store_reg_p,
! enum machine_mode *mode_out, HOST_WIDE_INT *offset_out)
{
enum machine_mode mode;
! if (expr == NULL || !track_expr_p (expr))
! return false;
!
! /* If REG was a paradoxical subreg, its REG_ATTRS will describe the
! whole subreg, but only the old inner part is really relevant. */
! mode = GET_MODE (loc);
! if (REG_P (loc) && !HARD_REGISTER_NUM_P (ORIGINAL_REGNO (loc)))
{
enum machine_mode pseudo_mode;
! pseudo_mode = PSEUDO_REGNO_MODE (ORIGINAL_REGNO (loc));
if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (pseudo_mode))
! {
! offset += byte_lowpart_offset (pseudo_mode, mode);
! mode = pseudo_mode;
! }
! }
!
! /* If LOC is a paradoxical lowpart of EXPR, refer to EXPR itself.
! Do the same if we are storing to a register and EXPR occupies
! the whole of register LOC; in that case, the whole of EXPR is
! being changed. We exclude complex modes from the second case
! because the real and imaginary parts are represented as separate
! pseudo registers, even if the whole complex value fits into one
! hard register. */
! if ((GET_MODE_SIZE (mode) > GET_MODE_SIZE (DECL_MODE (expr))
! || (store_reg_p
! && !COMPLEX_MODE_P (DECL_MODE (expr))
! && hard_regno_nregs[REGNO (loc)][DECL_MODE (expr)] == 1))
! && offset + byte_lowpart_offset (DECL_MODE (expr), mode) == 0)
! {
! mode = DECL_MODE (expr);
! offset = 0;
}
!
! if (!offset_valid_for_tracked_p (offset))
! return false;
!
! if (mode_out)
! *mode_out = mode;
! if (offset_out)
! *offset_out = offset;
! return true;
}
/* Return the MODE lowpart of LOC, or null if LOC is not something we
*************** mode_for_reg_attrs (rtx reg)
*** 1722,1728 ****
static rtx
var_lowpart (enum machine_mode mode, rtx loc)
{
! unsigned int offset, regno;
if (!REG_P (loc) && !MEM_P (loc))
return NULL;
--- 1757,1763 ----
static rtx
var_lowpart (enum machine_mode mode, rtx loc)
{
! unsigned int offset, reg_offset, regno;
if (!REG_P (loc) && !MEM_P (loc))
return NULL;
*************** var_lowpart (enum machine_mode mode, rtx
*** 1730,1742 ****
if (GET_MODE (loc) == mode)
return loc;
! offset = subreg_lowpart_offset (mode, GET_MODE (loc));
if (MEM_P (loc))
return adjust_address_nv (loc, mode, offset);
regno = REGNO (loc) + subreg_regno_offset (REGNO (loc), GET_MODE (loc),
! offset, mode);
return gen_rtx_REG_offset (loc, mode, regno, offset);
}
--- 1765,1778 ----
if (GET_MODE (loc) == mode)
return loc;
! offset = byte_lowpart_offset (mode, GET_MODE (loc));
if (MEM_P (loc))
return adjust_address_nv (loc, mode, offset);
+ reg_offset = subreg_lowpart_offset (mode, GET_MODE (loc));
regno = REGNO (loc) + subreg_regno_offset (REGNO (loc), GET_MODE (loc),
! reg_offset, mode);
return gen_rtx_REG_offset (loc, mode, regno, offset);
}
*************** count_uses (rtx *loc, void *insn)
*** 1754,1762 ****
VTI (bb)->n_mos++;
}
else if (MEM_P (*loc)
! && MEM_EXPR (*loc)
! && track_expr_p (MEM_EXPR (*loc))
! && offset_valid_for_tracked_p (INT_MEM_OFFSET (*loc)))
{
VTI (bb)->n_mos++;
}
--- 1790,1797 ----
VTI (bb)->n_mos++;
}
else if (MEM_P (*loc)
! && track_loc_p (*loc, MEM_EXPR (*loc), INT_MEM_OFFSET (*loc),
! false, NULL, NULL))
{
VTI (bb)->n_mos++;
}
*************** count_stores (rtx loc, const_rtx expr AT
*** 1787,1803 ****
static int
add_uses (rtx *loc, void *insn)
{
if (REG_P (*loc))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
! if (REG_EXPR (*loc)
! && track_expr_p (REG_EXPR (*loc))
! && offset_valid_for_tracked_p (REG_OFFSET (*loc)))
{
mo->type = MO_USE;
! mo->u.loc = var_lowpart (mode_for_reg_attrs (*loc), *loc);
}
else
{
--- 1822,1839 ----
static int
add_uses (rtx *loc, void *insn)
{
+ enum machine_mode mode;
+
if (REG_P (*loc))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
! if (track_loc_p (*loc, REG_EXPR (*loc), REG_OFFSET (*loc),
! false, &mode, NULL))
{
mo->type = MO_USE;
! mo->u.loc = var_lowpart (mode, *loc);
}
else
{
*************** add_uses (rtx *loc, void *insn)
*** 1807,1821 ****
mo->insn = (rtx) insn;
}
else if (MEM_P (*loc)
! && MEM_EXPR (*loc)
! && track_expr_p (MEM_EXPR (*loc))
! && offset_valid_for_tracked_p (INT_MEM_OFFSET (*loc)))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
mo->type = MO_USE;
! mo->u.loc = *loc;
mo->insn = (rtx) insn;
}
--- 1843,1856 ----
mo->insn = (rtx) insn;
}
else if (MEM_P (*loc)
! && track_loc_p (*loc, MEM_EXPR (*loc), INT_MEM_OFFSET (*loc),
! false, &mode, NULL))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
mo->type = MO_USE;
! mo->u.loc = var_lowpart (mode, *loc);
mo->insn = (rtx) insn;
}
*************** add_uses_1 (rtx *x, void *insn)
*** 1837,1858 ****
static void
add_stores (rtx loc, const_rtx expr, void *insn)
{
if (REG_P (loc))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
if (GET_CODE (expr) == CLOBBER
! || !(REG_EXPR (loc)
! && track_expr_p (REG_EXPR (loc))
! && offset_valid_for_tracked_p (REG_OFFSET (loc))))
{
mo->type = MO_CLOBBER;
mo->u.loc = loc;
}
else
{
- enum machine_mode mode = mode_for_reg_attrs (loc);
rtx src = NULL;
if (GET_CODE (expr) == SET && SET_DEST (expr) == loc)
--- 1872,1893 ----
static void
add_stores (rtx loc, const_rtx expr, void *insn)
{
+ enum machine_mode mode;
+
if (REG_P (loc))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
if (GET_CODE (expr) == CLOBBER
! || !track_loc_p (loc, REG_EXPR (loc), REG_OFFSET (loc),
! true, &mode, NULL))
{
mo->type = MO_CLOBBER;
mo->u.loc = loc;
}
else
{
rtx src = NULL;
if (GET_CODE (expr) == SET && SET_DEST (expr) == loc)
*************** add_stores (rtx loc, const_rtx expr, voi
*** 1878,1886 ****
mo->insn = (rtx) insn;
}
else if (MEM_P (loc)
! && MEM_EXPR (loc)
! && track_expr_p (MEM_EXPR (loc))
! && offset_valid_for_tracked_p (INT_MEM_OFFSET (loc)))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
--- 1913,1920 ----
mo->insn = (rtx) insn;
}
else if (MEM_P (loc)
! && track_loc_p (loc, MEM_EXPR (loc), INT_MEM_OFFSET (loc),
! false, &mode, NULL))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
*************** add_stores (rtx loc, const_rtx expr, voi
*** 1888,1901 ****
if (GET_CODE (expr) == CLOBBER)
{
mo->type = MO_CLOBBER;
! mo->u.loc = loc;
}
else
{
rtx src = NULL;
if (GET_CODE (expr) == SET && SET_DEST (expr) == loc)
! src = var_lowpart (GET_MODE (loc), SET_SRC (expr));
if (src == NULL)
{
--- 1922,1936 ----
if (GET_CODE (expr) == CLOBBER)
{
mo->type = MO_CLOBBER;
! mo->u.loc = var_lowpart (mode, loc);
}
else
{
rtx src = NULL;
if (GET_CODE (expr) == SET && SET_DEST (expr) == loc)
! src = var_lowpart (mode, SET_SRC (expr));
! loc = var_lowpart (mode, loc);
if (src == NULL)
{
*************** add_stores (rtx loc, const_rtx expr, voi
*** 1904,1909 ****
--- 1939,1946 ----
}
else
{
+ if (SET_SRC (expr) != src)
+ expr = gen_rtx_SET (VOIDmode, loc, src);
if (same_variable_part_p (SET_SRC (expr),
MEM_EXPR (loc),
INT_MEM_OFFSET (loc)))
*************** vt_add_function_parameters (void)
*** 3115,3120 ****
--- 3152,3158 ----
rtx decl_rtl = DECL_RTL_IF_SET (parm);
rtx incoming = DECL_INCOMING_RTL (parm);
tree decl;
+ enum machine_mode mode;
HOST_WIDE_INT offset;
dataflow_set *out;
*************** vt_add_function_parameters (void)
*** 3131,3148 ****
continue;
if (!vt_get_decl_and_offset (incoming, &decl, &offset))
! if (!vt_get_decl_and_offset (decl_rtl, &decl, &offset))
! continue;
if (!decl)
continue;
gcc_assert (parm == decl);
out = &VTI (ENTRY_BLOCK_PTR)->out;
if (REG_P (incoming))
{
gcc_assert (REGNO (incoming) < FIRST_PSEUDO_REGISTER);
attrs_list_insert (&out->regs[REGNO (incoming)],
parm, offset, incoming);
--- 3169,3194 ----
continue;
if (!vt_get_decl_and_offset (incoming, &decl, &offset))
! {
! if (!vt_get_decl_and_offset (decl_rtl, &decl, &offset))
! continue;
! offset += byte_lowpart_offset (GET_MODE (incoming),
! GET_MODE (decl_rtl));
! }
if (!decl)
continue;
gcc_assert (parm == decl);
+ if (!track_loc_p (incoming, parm, offset, false, &mode, &offset))
+ continue;
+
out = &VTI (ENTRY_BLOCK_PTR)->out;
if (REG_P (incoming))
{
+ incoming = var_lowpart (mode, incoming);
gcc_assert (REGNO (incoming) < FIRST_PSEUDO_REGISTER);
attrs_list_insert (&out->regs[REGNO (incoming)],
parm, offset, incoming);
*************** vt_add_function_parameters (void)
*** 3150,3157 ****
NULL);
}
else if (MEM_P (incoming))
! set_variable_part (out, incoming, parm, offset, VAR_INIT_STATUS_INITIALIZED,
! NULL);
}
}
--- 3196,3206 ----
NULL);
}
else if (MEM_P (incoming))
! {
! incoming = var_lowpart (mode, incoming);
! set_variable_part (out, incoming, parm, offset,
! VAR_INIT_STATUS_INITIALIZED, NULL);
! }
}
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* RFA: Fix tracking of pass-by-reference parameters
2007-11-15 12:31 ` Richard Sandiford
@ 2007-11-15 12:44 ` Richard Sandiford
2007-12-14 15:38 ` Eric Botcazou
2007-12-16 12:44 ` Fix PR rtl-optimization/33822 Eric Botcazou
1 sibling, 1 reply; 10+ messages in thread
From: Richard Sandiford @ 2007-11-15 12:44 UTC (permalink / raw)
To: gcc-patches
This is the follow-up to:
http://gcc.gnu.org/ml/gcc-patches/2007-11/msg00835.html
As explained in that message, if the ABI requires an argument to
be passed by reference, we attach register attributes to the pointer
and memory attributes to the dereferenced pointer. The debugging
output then treats the pointer as part of the deferenced value.
Bootstrapped & regression-tested on x86_64-linux-gnu. Also
regression-tested on mipsisa64-elf, where it fixes the debug
output for at least:
> - gcc.c-torture/compile/941019-1.c
> - gcc.c-torture/compile/vector-2.c
For example, in gcc.c-torture/compile/941019-1.c, we said that the
real part of "cld" was in (reg:DI $4) and the imaginary part was in
(mem:DI (plus:DI (reg:DI $4) (const_int 4))). We now correctly say
that the whole of cld is at (mem:CDI (reg:DI $4)).
OK to install?
Richard
gcc/
* tree.h (set_decl_incoming_rtl): Add a by_reference_p parameter.
* emit-rtl.c (set_decl_incoming_rtl): Likewise. Don't set the
rtl's register attributes when the parameter is true.
* function.c (assign_parms_unsplit_complex, assign_parms)
(expand_function_start): Update calls to set_decl_incoming_rtl.
Index: gcc/tree.h
===================================================================
--- gcc/tree.h 2007-11-14 21:45:41.000000000 +0000
+++ gcc/tree.h 2007-11-14 21:46:47.000000000 +0000
@@ -5130,7 +5130,7 @@ #define walk_tree_without_duplicates(a,b
/* Assign the RTX to declaration. */
extern void set_decl_rtl (tree, rtx);
-extern void set_decl_incoming_rtl (tree, rtx);
+extern void set_decl_incoming_rtl (tree, rtx, bool);
\f
/* Enum and arrays used for tree allocation stats.
Keep in sync with tree.c:tree_node_kind_names. */
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2007-11-14 21:45:40.000000000 +0000
+++ gcc/emit-rtl.c 2007-11-14 21:46:47.000000000 +0000
@@ -1044,13 +1044,14 @@ set_decl_rtl (tree t, rtx x)
set_reg_attrs_for_decl_rtl (t, x);
}
-/* Assign the RTX X to parameter declaration T. */
+/* Assign the RTX X to parameter declaration T. BY_REFERENCE_P is true
+ if the ABI requires the parameter to be passed by reference. */
void
-set_decl_incoming_rtl (tree t, rtx x)
+set_decl_incoming_rtl (tree t, rtx x, bool by_reference_p)
{
DECL_INCOMING_RTL (t) = x;
- if (x)
+ if (x && !by_reference_p)
set_reg_attrs_for_decl_rtl (t, x);
}
Index: gcc/function.c
===================================================================
--- gcc/function.c 2007-11-14 21:45:40.000000000 +0000
+++ gcc/function.c 2007-11-14 21:46:47.000000000 +0000
@@ -2970,13 +2970,13 @@ assign_parms_unsplit_complex (struct ass
imag = gen_lowpart_SUBREG (inner, imag);
}
tmp = gen_rtx_CONCAT (DECL_MODE (parm), real, imag);
- set_decl_incoming_rtl (parm, tmp);
+ set_decl_incoming_rtl (parm, tmp, false);
fnargs = TREE_CHAIN (fnargs);
}
else
{
SET_DECL_RTL (parm, DECL_RTL (fnargs));
- set_decl_incoming_rtl (parm, DECL_INCOMING_RTL (fnargs));
+ set_decl_incoming_rtl (parm, DECL_INCOMING_RTL (fnargs), false);
/* Set MEM_EXPR to the original decl, i.e. to PARM,
instead of the copy of decl, i.e. FNARGS. */
@@ -3032,7 +3032,7 @@ assign_parms (tree fndecl)
}
/* Record permanently how this parm was passed. */
- set_decl_incoming_rtl (parm, data.entry_parm);
+ set_decl_incoming_rtl (parm, data.entry_parm, data.passed_pointer);
/* Update info on where next arg arrives in registers. */
FUNCTION_ARG_ADVANCE (all.args_so_far, data.promoted_mode,
@@ -4252,7 +4252,7 @@ expand_function_start (tree subr)
tree parm = cfun->static_chain_decl;
rtx local = gen_reg_rtx (Pmode);
- set_decl_incoming_rtl (parm, static_chain_incoming_rtx);
+ set_decl_incoming_rtl (parm, static_chain_incoming_rtx, false);
SET_DECL_RTL (parm, local);
mark_reg_pointer (local, TYPE_ALIGN (TREE_TYPE (TREE_TYPE (parm))));
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: RFA: Fix tracking of pass-by-reference parameters
2007-11-15 12:44 ` RFA: Fix tracking of pass-by-reference parameters Richard Sandiford
@ 2007-12-14 15:38 ` Eric Botcazou
0 siblings, 0 replies; 10+ messages in thread
From: Eric Botcazou @ 2007-12-14 15:38 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
[stretching a bit the purview of my maintainership...]
> * tree.h (set_decl_incoming_rtl): Add a by_reference_p parameter.
> * emit-rtl.c (set_decl_incoming_rtl): Likewise. Don't set the
> rtl's register attributes when the parameter is true.
> * function.c (assign_parms_unsplit_complex, assign_parms)
> (expand_function_start): Update calls to set_decl_incoming_rtl.
OK, thanks.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fix PR rtl-optimization/33822
2007-11-15 12:31 ` Richard Sandiford
2007-11-15 12:44 ` RFA: Fix tracking of pass-by-reference parameters Richard Sandiford
@ 2007-12-16 12:44 ` Eric Botcazou
2007-12-18 10:47 ` Richard Sandiford
1 sibling, 1 reply; 10+ messages in thread
From: Eric Botcazou @ 2007-12-16 12:44 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> So, going back to the original REG_OFFSET question, I think it would make
> more sense for the REG_OFFSET of (reg:M R) to be the offset of the first
> byte in the M-mode value. That's what MEM_OFFSET is -- more later about
> why we need negative offsets there too -- and var-tracking.c fundamentally
> assumes that memory offsets and register offsets are comparable.
This makes sense, I think.
> Luckily, the code that sets REG_OFFSET is localised to emit-rtl.c, with
> the rest of the compiler using higher-level interfaces. (Thanks Jan!)
> So it's easy to enforce a consistent representation. Also, the problem
> identified in PR 14084 resurfaces if you revert the associated patch,
> so it's easy to verify that this alternative approach fixes it.
>
> So, the patch below:
>
> - Removes the 14084 code.
You need to update the head comment of the function.
> - Adds a byte_lowpart_offset function to go alongside
> subreg_lowpart_offset. Like subreg_lowpart_offset, this new function
> copes with identity lowparts, true lowparts and paradoxical lowparts.
> The offset can be negative for paradoxical lowparts on big-endian
> systems.
I think that we should make it explicit (in the head comment of the functions
for example) that subreg_lowpart_offset is meant for SUBREG_BYTE and the new
byte_lowpart_offset is meant for REG_OFFSET.
> - Makes sure that an M-mode REG for an N-mode decl starts off
> with a REG_OFFSET of "byte_lowpart (M, N)".
Right, so don't we need to do something for gen_rtx_REG_offset and
gen_reg_rtx_offset, at least document what type of offsets they expect?
You have updated one caller (var_lowpart), what about the others? In
particular, it seems to me that simplify_subreg already computes the
byte_lowpart_offset manually before invoking gen_rtx_REG_offset.
> And that's the end of this particular story as far as recording
> the offsets goes. There are also some problems in the way
> var-tracking uses them:
>
> - My original patch for paradoxical lowparts wasn't aggressive enough.
> We also need to cope with promoted variables, such as 32-bit variables
> stored in 64-bit pseudo registers.
>
> - We need to do the same when setting up the locations of
> promoted parameters.
>
> - We need to do the same when a 64-bit promoted variable is spilled
> to the stack. We currently emit a memory location for the start
> of the spill slot, which is wrong on big-endian targets when the
> slot is padded.
>
> - We currently assume that a store to a register that holds VAR+OFFSET
> does not clobber [VAR+0, VAR+OFFSET). That's wrong when the register
> is big enough to hold the whole of VAR. E.g. it's perfectly possible
> for a target to store to (reg:SI R) and then reference the value using
> (reg:DI R), where (reg:DI R) is a single register.
>
> Again, these are all problems that lead to wrong debug info. And luckily,
> they're not really separate problems at all. The idea is that if we see
> a reference to VAR with an offset consistent with a lowpart of VAR and
> either:
>
> 1) that lowpart is paradoxical; or
> 2) we're storing into a register that can hold the whole of VAR
>
> then we should consider this to be an access to the whole of VAR.
>
> After those changes, we can enforce the [0, MAX_VAR_PARTS) range,
> thus avoiding the problem that sparked my original reply.
> (Rejecting negative offsets has other benefits too; see later.)
If you have eliminated all but one invocation of offset_valid_for_tracked_p,
just integrate the function into its only caller.
> Now I realise that the patch might be a bit daunting, but I think it's
> actually very localised, and it's all bug fixes. I really do think it's
> stage 3 material.
OK, it should indeed be safe.
> However, because it isn't exactly a one-liner either, I've tried to go an
> extra mile as far as testing goes.
That's an understatement, more like a full marathon in my opinion. :-)
Thanks for conduction such an extensive testing.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fix PR rtl-optimization/33822
2007-12-16 12:44 ` Fix PR rtl-optimization/33822 Eric Botcazou
@ 2007-12-18 10:47 ` Richard Sandiford
2007-12-18 18:57 ` Eric Botcazou
0 siblings, 1 reply; 10+ messages in thread
From: Richard Sandiford @ 2007-12-18 10:47 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 6452 bytes --]
Thanks for reviewing all that lot.
Eric Botcazou <ebotcazou@libertysurf.fr> writes:
>> So, the patch below:
>>
>> - Removes the 14084 code.
>
> You need to update the head comment of the function.
Oops. Now done.
>> - Adds a byte_lowpart_offset function to go alongside
>> subreg_lowpart_offset. Like subreg_lowpart_offset, this new function
>> copes with identity lowparts, true lowparts and paradoxical lowparts.
>> The offset can be negative for paradoxical lowparts on big-endian
>> systems.
>
> I think that we should make it explicit (in the head comment of the
> functions for example) that subreg_lowpart_offset is meant for
> SUBREG_BYTE and the new byte_lowpart_offset is meant for REG_OFFSET.
OK. I'm a little reluctant to mention REG_OFFSET explicitly in the
comment above byte_lowpart_offset, because the function applies to
MEM_OFFSETs too, and could easily be used in other situations.
So instead I tried to be more explicit about the way
byte_lowpart_offset calculates the offset.
I added a reference to SUBREG_BYTE above subreg_lowpart_offset.
I added a note above reg_attrs to say that the offset is calculated
in the same way as mem_attrs.
I think rtl.texi already describes the SUBREG_BYTE calculation in enough
detail, but I noticed that rtl.def was years out of date. I think that
shows we should have detailed documentation in one place only (rtl.texi),
so I simplified the rtl.def version.
>> - Makes sure that an M-mode REG for an N-mode decl starts off
>> with a REG_OFFSET of "byte_lowpart (M, N)".
>
> Right, so don't we need to do something for gen_rtx_REG_offset and
> gen_reg_rtx_offset, at least document what type of offsets they expect?
>
> You have updated one caller (var_lowpart), what about the others? In
> particular, it seems to me that simplify_subreg already computes the
> byte_lowpart_offset manually before invoking gen_rtx_REG_offset.
Yeah, I agree that simplify_subreg and gen_rtx_REG_offset were already
consistent. (That was actually one of the reasons I went the way I did.
Sorry for not making that clear... too much to say!)
simplify_subreg is the biggest caller of gen_rtx_REG_offset, so my take
was that gen_rtx_REG_offset has taken a MEM_OFFSET-type argument ever
since the 14084 patch. I.e. the patch for that PR crystalised the
choice of offset parameter, and we're keeping it the same here.
(But given that the current code doesn't work for all cases, I suppose
you could argue it either way. And the doubt about the current
interface certainly lends weight to what you say about more
documentation.)
So responding to the first paragraph: the gen_reg_rtx_offset caller
in lower-subreg.c is OK, as it is dealing with word_mode subregs of
a multiword value. There are no paradoxical offsets.
The call to gen_rtx_REG_offset in final.c does look suspicious.
(That whole "generate a REG even if simplify_subreg couldn't" code
seems odd. I wonder when it triggers these days?) I've also fixed
the ia64.c caller.
I've made the comments for update_reg_offset, gen_rtx_REG_offset
and gen_reg_rtx_offset say "with OFFSET added to the REG_OFFSET"
rather than "offsetted by OFFSET". I think this, in combination
with the reg_attrs comment, makes the type of offset more explicit.
>> After those changes, we can enforce the [0, MAX_VAR_PARTS) range,
>> thus avoiding the problem that sparked my original reply.
>> (Rejecting negative offsets has other benefits too; see later.)
>
> If you have eliminated all but one invocation of offset_valid_for_tracked_p,
> just integrate the function into its only caller.
OK.
I've attached the new patch below, along with an interdiff of the new
patch and the old one. This shows that I sent the wrong version of
the original patch, with a stupid thinko in the reload.c hunk. Sorry!
Bootstrapped & regression-tested on x86_64-linux-gnu. Regression-tested
on mipsisa64-elfoabi. I also built cc1 for ia64-linux-gnu and checked
that there were no warnings or errors for ia64.o. OK to install?
Richard
gcc/
* rtl.def (SUBREG): Update comments.
* rtl.h (reg_attrs): Be explicit about the type of offset used.
(set_reg_attrs_from_mem): Rename to...
(set_reg_attrs_from_value): ...this.
(adjust_reg_mode, byte_lowpart_offset): Declare.
* emit-rtl.c (byte_lowpart_offset): New function.
(update_reg_offset): Remove special offset handling for big-endian
targets.
(gen_rtx_REG_offset, gen_reg_rtx_offset): Explicitly say that the
offset parameter is added to REG_OFFSET.
(adjust_reg_mode): New function.
(set_reg_attrs_for_mem): Rename to...
(set_reg_attrs_for_value): ...this and generalize to all values.
If the register is a lowpart of the value, adjust the offset
accordingly.
(set_reg_attrs_for_parm): Update after the above renaming.
(set_reg_attrs_for_decl_rtl): New function, split out from
set_decl_incoming_rtl. Set the offset of plain REGs to the
offset of the REG's mode from the decl's. Assert that all
subregs are lowparts and handle their inner registers in the
same way as plain REGs.
(set_decl_rtl, set_incoming_decl_rtl): Use reg_attrs_for_decl_rtl.
(subreg_lowpart_offset): Explicitly say that the returned offset
is a SUBREG_BYTE.
* combine.c (do_SUBST_MODE, try_combine, undo_all): Use adjust_reg_mode
instead of PUT_MODE.
* final.c (alter_subreg): Fix/update argument to gen_rtx_REG_offset.
* config/ia64/ia64.c (ia64_expand_load_address): Likewise.
* regclass.c (reg_scan_mark_refs): Use set_reg_attrs_from_value.
* reload.c (find_reloads_subreg_address): Call set_mem_offset
when offseting a MEM.
* var-tracking.c (offset_valid_for_tracked_p): Delete.
(mode_for_reg_attrs): Replace with...
(track_loc_p): ...this new function. Return the mode and offset
to the caller, checking that the latter is valid. If the rtx is
a paradoxical lowpart of the decl, use the decl's mode instead.
Do the same when storing to a register that contains the entire decl.
(var_lowpart): Use byte_lowpart_offset rather than
subreg_lowpart_offset when adjusting the offset attribute.
(count_uses, add_uses, add_stores): Use track_reg_p instead of
REG_EXPR, MEM_EXPR, REG_OFFSET, INT_MEM_OFFSET, track_expr_p,
offset_valid_for_tracked_p and mode_for_reg_attrs. Generate
lowparts for MEMs as well as REGs.
(vt_add_function_parameters): When obtaining the information from
the decl_rtl, adjust the offset to match incoming. Use track_loc_p
and var_lowpart.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: new.diff --]
[-- Type: text/x-diff, Size: 25490 bytes --]
Index: gcc/rtl.def
===================================================================
--- gcc/rtl.def 2007-12-18 07:39:35.000000000 +0000
+++ gcc/rtl.def 2007-12-18 07:48:07.000000000 +0000
@@ -371,14 +371,8 @@ DEF_RTL_EXPR(REG, "reg", "i00", RTX_OBJ)
marked as having one operand so it can be turned into a REG. */
DEF_RTL_EXPR(SCRATCH, "scratch", "0", RTX_OBJ)
-/* One word of a multi-word value.
- The first operand is the complete value; the second says which word.
- The WORDS_BIG_ENDIAN flag controls whether word number 0
- (as numbered in a SUBREG) is the most or least significant word.
-
- This is also used to refer to a value in a different machine mode.
- For example, it can be used to refer to a SImode value as if it were
- Qimode, or vice versa. Then the word number is always 0. */
+/* A reference to a part of another value. The first operand is the
+ complete value and the second is the byte offset of the selected part. */
DEF_RTL_EXPR(SUBREG, "subreg", "ei", RTX_EXTRA)
/* This one-argument rtx is used for move instructions
Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h 2007-12-18 07:48:06.000000000 +0000
+++ gcc/rtl.h 2007-12-18 07:48:07.000000000 +0000
@@ -149,7 +149,11 @@ typedef struct mem_attrs GTY(())
} mem_attrs;
/* Structure used to describe the attributes of a REG in similar way as
- mem_attrs does for MEM above. */
+ mem_attrs does for MEM above. Note that the OFFSET field is calculated
+ in the same way as for mem_attrs, rather than in the same way as a
+ SUBREG_BYTE. For example, if a big-endian target stores a byte
+ object in the low part of a 4-byte register, the OFFSET field
+ will be -3 rather than 0. */
typedef struct reg_attrs GTY(())
{
@@ -1476,9 +1480,10 @@ extern rtx copy_insn_1 (rtx);
extern rtx copy_insn (rtx);
extern rtx gen_int_mode (HOST_WIDE_INT, enum machine_mode);
extern rtx emit_copy_of_insn_after (rtx, rtx);
-extern void set_reg_attrs_from_mem (rtx, rtx);
+extern void set_reg_attrs_from_value (rtx, rtx);
extern void set_mem_attrs_from_reg (rtx, rtx);
extern void set_reg_attrs_for_parm (rtx, rtx);
+extern void adjust_reg_mode (rtx, enum machine_mode);
extern int mem_expr_equal_p (const_tree, const_tree);
/* In rtl.c */
@@ -1522,6 +1527,7 @@ extern unsigned int subreg_lowpart_offse
enum machine_mode);
extern unsigned int subreg_highpart_offset (enum machine_mode,
enum machine_mode);
+extern int byte_lowpart_offset (enum machine_mode, enum machine_mode);
extern rtx make_safe_from (rtx, rtx);
extern rtx convert_memory_address (enum machine_mode, rtx);
extern rtx get_insns (void);
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2007-12-18 07:48:06.000000000 +0000
+++ gcc/emit-rtl.c 2007-12-18 09:25:48.000000000 +0000
@@ -837,6 +837,22 @@ gen_rtvec_v (int n, rtx *argp)
return rt_val;
}
\f
+/* Return the number of bytes between the start of an OUTER_MODE
+ in-memory value and the start of an INNER_MODE in-memory value,
+ given that the former is a lowpart of the latter. It may be a
+ paradoxical lowpart, in which case the offset will be negative
+ on big-endian targets. */
+
+int
+byte_lowpart_offset (enum machine_mode outer_mode,
+ enum machine_mode inner_mode)
+{
+ if (GET_MODE_SIZE (outer_mode) < GET_MODE_SIZE (inner_mode))
+ return subreg_lowpart_offset (outer_mode, inner_mode);
+ else
+ return -subreg_lowpart_offset (inner_mode, outer_mode);
+}
+\f
/* Generate a REG rtx for a new pseudo register of mode MODE.
This pseudo is assigned the next sequential register number. */
@@ -891,101 +907,18 @@ gen_reg_rtx (enum machine_mode mode)
return val;
}
-/* Update NEW with the same attributes as REG, but offsetted by OFFSET.
- Do the big endian correction if needed. */
+/* Update NEW with the same attributes as REG, but with OFFSET added
+ to the REG_OFFSET. */
static void
update_reg_offset (rtx new, rtx reg, int offset)
{
- tree decl;
- HOST_WIDE_INT var_size;
-
- /* PR middle-end/14084
- The problem appears when a variable is stored in a larger register
- and later it is used in the original mode or some mode in between
- or some part of variable is accessed.
-
- On little endian machines there is no problem because
- the REG_OFFSET of the start of the variable is the same when
- accessed in any mode (it is 0).
-
- However, this is not true on big endian machines.
- The offset of the start of the variable is different when accessed
- in different modes.
- When we are taking a part of the REG we have to change the OFFSET
- from offset WRT size of mode of REG to offset WRT size of variable.
-
- If we would not do the big endian correction the resulting REG_OFFSET
- would be larger than the size of the DECL.
-
- Examples of correction, for BYTES_BIG_ENDIAN WORDS_BIG_ENDIAN machine:
-
- REG.mode MODE DECL size old offset new offset description
- DI SI 4 4 0 int32 in SImode
- DI SI 1 4 0 char in SImode
- DI QI 1 7 0 char in QImode
- DI QI 4 5 1 1st element in QImode
- of char[4]
- DI HI 4 6 2 1st element in HImode
- of int16[2]
-
- If the size of DECL is equal or greater than the size of REG
- we can't do this correction because the register holds the
- whole variable or a part of the variable and thus the REG_OFFSET
- is already correct. */
-
- decl = REG_EXPR (reg);
- if ((BYTES_BIG_ENDIAN || WORDS_BIG_ENDIAN)
- && decl != NULL
- && offset > 0
- && GET_MODE_SIZE (GET_MODE (reg)) > GET_MODE_SIZE (GET_MODE (new))
- && ((var_size = int_size_in_bytes (TREE_TYPE (decl))) > 0
- && var_size < GET_MODE_SIZE (GET_MODE (reg))))
- {
- int offset_le;
-
- /* Convert machine endian to little endian WRT size of mode of REG. */
- if (WORDS_BIG_ENDIAN)
- offset_le = ((GET_MODE_SIZE (GET_MODE (reg)) - 1 - offset)
- / UNITS_PER_WORD) * UNITS_PER_WORD;
- else
- offset_le = (offset / UNITS_PER_WORD) * UNITS_PER_WORD;
-
- if (BYTES_BIG_ENDIAN)
- offset_le += ((GET_MODE_SIZE (GET_MODE (reg)) - 1 - offset)
- % UNITS_PER_WORD);
- else
- offset_le += offset % UNITS_PER_WORD;
-
- if (offset_le >= var_size)
- {
- /* MODE is wider than the variable so the new reg will cover
- the whole variable so the resulting OFFSET should be 0. */
- offset = 0;
- }
- else
- {
- /* Convert little endian to machine endian WRT size of variable. */
- if (WORDS_BIG_ENDIAN)
- offset = ((var_size - 1 - offset_le)
- / UNITS_PER_WORD) * UNITS_PER_WORD;
- else
- offset = (offset_le / UNITS_PER_WORD) * UNITS_PER_WORD;
-
- if (BYTES_BIG_ENDIAN)
- offset += ((var_size - 1 - offset_le)
- % UNITS_PER_WORD);
- else
- offset += offset_le % UNITS_PER_WORD;
- }
- }
-
REG_ATTRS (new) = get_reg_attrs (REG_EXPR (reg),
REG_OFFSET (reg) + offset);
}
-/* Generate a register with same attributes as REG, but offsetted by
- OFFSET. */
+/* Generate a register with same attributes as REG, but with OFFSET
+ added to the REG_OFFSET. */
rtx
gen_rtx_REG_offset (rtx reg, enum machine_mode mode, unsigned int regno,
@@ -998,7 +931,7 @@ gen_rtx_REG_offset (rtx reg, enum machin
}
/* Generate a new pseudo-register with the same attributes as REG, but
- offsetted by OFFSET. */
+ with OFFSET added to the REG_OFFSET. */
rtx
gen_reg_rtx_offset (rtx reg, enum machine_mode mode, int offset)
@@ -1009,14 +942,30 @@ gen_reg_rtx_offset (rtx reg, enum machin
return new;
}
-/* Set REG to the decl that MEM refers to. */
+/* Adjust REG in-place so that it has mode MODE. It is assumed that the
+ new register is a (possibly paradoxical) lowpart of the old one. */
+
+void
+adjust_reg_mode (rtx reg, enum machine_mode mode)
+{
+ update_reg_offset (reg, reg, byte_lowpart_offset (mode, GET_MODE (reg)));
+ PUT_MODE (reg, mode);
+}
+
+/* Copy REG's attributes from X, if X has any attributes. If REG and X
+ have different modes, REG is a (possibly paradoxical) lowpart of X. */
void
-set_reg_attrs_from_mem (rtx reg, rtx mem)
+set_reg_attrs_from_value (rtx reg, rtx x)
{
- if (MEM_OFFSET (mem) && GET_CODE (MEM_OFFSET (mem)) == CONST_INT)
+ int offset;
+
+ offset = byte_lowpart_offset (GET_MODE (reg), GET_MODE (x));
+ if (MEM_P (x) && MEM_OFFSET (x) && GET_CODE (MEM_OFFSET (x)) == CONST_INT)
REG_ATTRS (reg)
- = get_reg_attrs (MEM_EXPR (mem), INTVAL (MEM_OFFSET (mem)));
+ = get_reg_attrs (MEM_EXPR (x), INTVAL (MEM_OFFSET (x)) + offset);
+ if (REG_P (x) && REG_ATTRS (x))
+ update_reg_offset (reg, x, offset);
}
/* Set the register attributes for registers contained in PARM_RTX.
@@ -1026,7 +975,7 @@ set_reg_attrs_from_mem (rtx reg, rtx mem
set_reg_attrs_for_parm (rtx parm_rtx, rtx mem)
{
if (REG_P (parm_rtx))
- set_reg_attrs_from_mem (parm_rtx, mem);
+ set_reg_attrs_from_value (parm_rtx, mem);
else if (GET_CODE (parm_rtx) == PARALLEL)
{
/* Check for a NULL entry in the first slot, used to indicate that the
@@ -1043,54 +992,21 @@ set_reg_attrs_for_parm (rtx parm_rtx, rt
}
}
-/* Assign the RTX X to declaration T. */
-void
-set_decl_rtl (tree t, rtx x)
-{
- DECL_WRTL_CHECK (t)->decl_with_rtl.rtl = x;
+/* Set the REG_ATTRS for registers in value X, given that X represents
+ decl T. */
- if (!x)
- return;
- /* For register, we maintain the reverse information too. */
- if (REG_P (x))
- REG_ATTRS (x) = get_reg_attrs (t, 0);
- else if (GET_CODE (x) == SUBREG)
- REG_ATTRS (SUBREG_REG (x))
- = get_reg_attrs (t, -SUBREG_BYTE (x));
- if (GET_CODE (x) == CONCAT)
- {
- if (REG_P (XEXP (x, 0)))
- REG_ATTRS (XEXP (x, 0)) = get_reg_attrs (t, 0);
- if (REG_P (XEXP (x, 1)))
- REG_ATTRS (XEXP (x, 1))
- = get_reg_attrs (t, GET_MODE_UNIT_SIZE (GET_MODE (XEXP (x, 0))));
- }
- if (GET_CODE (x) == PARALLEL)
+static void
+set_reg_attrs_for_decl_rtl (tree t, rtx x)
+{
+ if (GET_CODE (x) == SUBREG)
{
- int i;
- for (i = 0; i < XVECLEN (x, 0); i++)
- {
- rtx y = XVECEXP (x, 0, i);
- if (REG_P (XEXP (y, 0)))
- REG_ATTRS (XEXP (y, 0)) = get_reg_attrs (t, INTVAL (XEXP (y, 1)));
- }
+ gcc_assert (subreg_lowpart_p (x));
+ x = SUBREG_REG (x);
}
-}
-
-/* Assign the RTX X to parameter declaration T. */
-void
-set_decl_incoming_rtl (tree t, rtx x)
-{
- DECL_INCOMING_RTL (t) = x;
-
- if (!x)
- return;
- /* For register, we maintain the reverse information too. */
if (REG_P (x))
- REG_ATTRS (x) = get_reg_attrs (t, 0);
- else if (GET_CODE (x) == SUBREG)
- REG_ATTRS (SUBREG_REG (x))
- = get_reg_attrs (t, -SUBREG_BYTE (x));
+ REG_ATTRS (x)
+ = get_reg_attrs (t, byte_lowpart_offset (GET_MODE (x),
+ TYPE_MODE (TREE_TYPE (t))));
if (GET_CODE (x) == CONCAT)
{
if (REG_P (XEXP (x, 0)))
@@ -1119,6 +1035,26 @@ set_decl_incoming_rtl (tree t, rtx x)
}
}
+/* Assign the RTX X to declaration T. */
+
+void
+set_decl_rtl (tree t, rtx x)
+{
+ DECL_WRTL_CHECK (t)->decl_with_rtl.rtl = x;
+ if (x)
+ set_reg_attrs_for_decl_rtl (t, x);
+}
+
+/* Assign the RTX X to parameter declaration T. */
+
+void
+set_decl_incoming_rtl (tree t, rtx x)
+{
+ DECL_INCOMING_RTL (t) = x;
+ if (x)
+ set_reg_attrs_for_decl_rtl (t, x);
+}
+
/* Identify REG (which may be a CONCAT) as a user register. */
void
@@ -1304,8 +1240,7 @@ gen_highpart_mode (enum machine_mode out
subreg_highpart_offset (outermode, innermode));
}
-/* Return offset in bytes to get OUTERMODE low part
- of the value in mode INNERMODE stored in memory in target format. */
+/* Return the SUBREG_BYTE for an OUTERMODE lowpart of an INNERMODE value. */
unsigned int
subreg_lowpart_offset (enum machine_mode outermode, enum machine_mode innermode)
Index: gcc/combine.c
===================================================================
--- gcc/combine.c 2007-12-18 07:48:06.000000000 +0000
+++ gcc/combine.c 2007-12-18 07:48:07.000000000 +0000
@@ -751,7 +751,7 @@ do_SUBST_MODE (rtx *into, enum machine_m
buf->kind = UNDO_MODE;
buf->where.r = into;
buf->old_contents.m = oldval;
- PUT_MODE (*into, newval);
+ adjust_reg_mode (*into, newval);
buf->next = undobuf.undos, undobuf.undos = buf;
}
@@ -2984,7 +2984,7 @@ try_combine (rtx i3, rtx i2, rtx i1, int
{
struct undo *buf;
- PUT_MODE (regno_reg_rtx[REGNO (i2dest)], old_mode);
+ adjust_reg_mode (regno_reg_rtx[REGNO (i2dest)], old_mode);
buf = undobuf.undos;
undobuf.undos = buf->next;
buf->next = undobuf.frees;
@@ -3826,7 +3826,7 @@ undo_all (void)
*undo->where.i = undo->old_contents.i;
break;
case UNDO_MODE:
- PUT_MODE (*undo->where.r, undo->old_contents.m);
+ adjust_reg_mode (*undo->where.r, undo->old_contents.m);
break;
default:
gcc_unreachable ();
Index: gcc/final.c
===================================================================
--- gcc/final.c 2007-12-18 07:39:35.000000000 +0000
+++ gcc/final.c 2007-12-18 07:48:07.000000000 +0000
@@ -2763,8 +2763,15 @@ alter_subreg (rtx *xp)
else if (REG_P (y))
{
/* Simplify_subreg can't handle some REG cases, but we have to. */
- unsigned int regno = subreg_regno (x);
- *xp = gen_rtx_REG_offset (y, GET_MODE (x), regno, SUBREG_BYTE (x));
+ unsigned int regno;
+ HOST_WIDE_INT offset;
+
+ regno = subreg_regno (x);
+ if (subreg_lowpart_p (x))
+ offset = byte_lowpart_offset (GET_MODE (x), GET_MODE (y));
+ else
+ offset = SUBREG_BYTE (x);
+ *xp = gen_rtx_REG_offset (y, GET_MODE (x), regno, offset);
}
}
Index: gcc/config/ia64/ia64.c
===================================================================
--- gcc/config/ia64/ia64.c 2007-12-18 09:26:01.000000000 +0000
+++ gcc/config/ia64/ia64.c 2007-12-18 09:26:22.000000000 +0000
@@ -796,7 +796,8 @@ ia64_expand_load_address (rtx dest, rtx
computation below are also more natural to compute as 64-bit quantities.
If we've been given an SImode destination register, change it. */
if (GET_MODE (dest) != Pmode)
- dest = gen_rtx_REG_offset (dest, Pmode, REGNO (dest), 0);
+ dest = gen_rtx_REG_offset (dest, Pmode, REGNO (dest),
+ byte_lowpart_offset (Pmode, GET_MODE (dest)));
if (TARGET_NO_PIC)
return false;
Index: gcc/regclass.c
===================================================================
--- gcc/regclass.c 2007-12-18 07:48:06.000000000 +0000
+++ gcc/regclass.c 2007-12-18 07:48:07.000000000 +0000
@@ -2435,10 +2435,7 @@ reg_scan_mark_refs (rtx x, rtx insn)
|| (GET_CODE (src) == SUBREG && subreg_lowpart_p (src)))
src = XEXP (src, 0);
- if (REG_P (src))
- REG_ATTRS (dest) = REG_ATTRS (src);
- if (MEM_P (src))
- set_reg_attrs_from_mem (dest, src);
+ set_reg_attrs_from_value (dest, src);
}
/* ... fall through ... */
Index: gcc/reload.c
===================================================================
--- gcc/reload.c 2007-12-18 07:48:06.000000000 +0000
+++ gcc/reload.c 2007-12-18 07:48:07.000000000 +0000
@@ -6025,6 +6025,8 @@ find_reloads_subreg_address (rtx x, int
XEXP (tem, 0) = plus_constant (XEXP (tem, 0), offset);
PUT_MODE (tem, GET_MODE (x));
+ if (MEM_OFFSET (tem))
+ set_mem_offset (tem, plus_constant (MEM_OFFSET (tem), offset));
/* If this was a paradoxical subreg that we replaced, the
resulting memory must be sufficiently aligned to allow
Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c 2007-12-18 07:48:06.000000000 +0000
+++ gcc/var-tracking.c 2007-12-18 07:48:07.000000000 +0000
@@ -1645,18 +1645,6 @@ track_expr_p (tree expr)
return 1;
}
-/* Return true if OFFSET is a valid offset for a register or memory
- access we want to track. This is used to reject out-of-bounds
- accesses that can cause assertions to fail later. Note that we
- don't reject negative offsets because they can be generated for
- paradoxical subregs on big-endian architectures. */
-
-static inline bool
-offset_valid_for_tracked_p (HOST_WIDE_INT offset)
-{
- return (-MAX_VAR_PARTS < offset) && (offset < MAX_VAR_PARTS);
-}
-
/* Determine whether a given LOC refers to the same variable part as
EXPR+OFFSET. */
@@ -1691,28 +1679,65 @@ same_variable_part_p (rtx loc, tree expr
return (expr == expr2 && offset == offset2);
}
-/* REG is a register we want to track. If not all of REG contains useful
- information, return the mode of the lowpart that does contain useful
- information, otherwise return the mode of REG.
+/* LOC is a REG or MEM that we would like to track if possible.
+ If EXPR is null, we don't know what expression LOC refers to,
+ otherwise it refers to EXPR + OFFSET. STORE_REG_P is true if
+ LOC is an lvalue register.
+
+ Return true if EXPR is nonnull and if LOC, or some lowpart of it,
+ is something we can track. When returning true, store the mode of
+ the lowpart we can track in *MODE_OUT (if nonnull) and its offset
+ from EXPR in *OFFSET_OUT (if nonnull). */
- If REG was a paradoxical subreg, its REG_ATTRS will describe the
- whole subreg, but only the old inner part is really relevant. */
-
-static enum machine_mode
-mode_for_reg_attrs (rtx reg)
+static bool
+track_loc_p (rtx loc, tree expr, HOST_WIDE_INT offset, bool store_reg_p,
+ enum machine_mode *mode_out, HOST_WIDE_INT *offset_out)
{
enum machine_mode mode;
- mode = GET_MODE (reg);
- if (!HARD_REGISTER_NUM_P (ORIGINAL_REGNO (reg)))
+ if (expr == NULL || !track_expr_p (expr))
+ return false;
+
+ /* If REG was a paradoxical subreg, its REG_ATTRS will describe the
+ whole subreg, but only the old inner part is really relevant. */
+ mode = GET_MODE (loc);
+ if (REG_P (loc) && !HARD_REGISTER_NUM_P (ORIGINAL_REGNO (loc)))
{
enum machine_mode pseudo_mode;
- pseudo_mode = PSEUDO_REGNO_MODE (ORIGINAL_REGNO (reg));
+ pseudo_mode = PSEUDO_REGNO_MODE (ORIGINAL_REGNO (loc));
if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (pseudo_mode))
- mode = pseudo_mode;
+ {
+ offset += byte_lowpart_offset (pseudo_mode, mode);
+ mode = pseudo_mode;
+ }
+ }
+
+ /* If LOC is a paradoxical lowpart of EXPR, refer to EXPR itself.
+ Do the same if we are storing to a register and EXPR occupies
+ the whole of register LOC; in that case, the whole of EXPR is
+ being changed. We exclude complex modes from the second case
+ because the real and imaginary parts are represented as separate
+ pseudo registers, even if the whole complex value fits into one
+ hard register. */
+ if ((GET_MODE_SIZE (mode) > GET_MODE_SIZE (DECL_MODE (expr))
+ || (store_reg_p
+ && !COMPLEX_MODE_P (DECL_MODE (expr))
+ && hard_regno_nregs[REGNO (loc)][DECL_MODE (expr)] == 1))
+ && offset + byte_lowpart_offset (DECL_MODE (expr), mode) == 0)
+ {
+ mode = DECL_MODE (expr);
+ offset = 0;
}
- return mode;
+
+ if (offset < 0 || offset >= MAX_VAR_PARTS)
+ return false;
+
+ if (mode_out)
+ *mode_out = mode;
+ if (offset_out)
+ *offset_out = offset;
+ return true;
}
/* Return the MODE lowpart of LOC, or null if LOC is not something we
@@ -1722,7 +1747,7 @@ mode_for_reg_attrs (rtx reg)
static rtx
var_lowpart (enum machine_mode mode, rtx loc)
{
- unsigned int offset, regno;
+ unsigned int offset, reg_offset, regno;
if (!REG_P (loc) && !MEM_P (loc))
return NULL;
@@ -1730,13 +1755,14 @@ var_lowpart (enum machine_mode mode, rtx
if (GET_MODE (loc) == mode)
return loc;
- offset = subreg_lowpart_offset (mode, GET_MODE (loc));
+ offset = byte_lowpart_offset (mode, GET_MODE (loc));
if (MEM_P (loc))
return adjust_address_nv (loc, mode, offset);
+ reg_offset = subreg_lowpart_offset (mode, GET_MODE (loc));
regno = REGNO (loc) + subreg_regno_offset (REGNO (loc), GET_MODE (loc),
- offset, mode);
+ reg_offset, mode);
return gen_rtx_REG_offset (loc, mode, regno, offset);
}
@@ -1754,9 +1780,8 @@ count_uses (rtx *loc, void *insn)
VTI (bb)->n_mos++;
}
else if (MEM_P (*loc)
- && MEM_EXPR (*loc)
- && track_expr_p (MEM_EXPR (*loc))
- && offset_valid_for_tracked_p (INT_MEM_OFFSET (*loc)))
+ && track_loc_p (*loc, MEM_EXPR (*loc), INT_MEM_OFFSET (*loc),
+ false, NULL, NULL))
{
VTI (bb)->n_mos++;
}
@@ -1787,17 +1812,18 @@ count_stores (rtx loc, const_rtx expr AT
static int
add_uses (rtx *loc, void *insn)
{
+ enum machine_mode mode;
+
if (REG_P (*loc))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
- if (REG_EXPR (*loc)
- && track_expr_p (REG_EXPR (*loc))
- && offset_valid_for_tracked_p (REG_OFFSET (*loc)))
+ if (track_loc_p (*loc, REG_EXPR (*loc), REG_OFFSET (*loc),
+ false, &mode, NULL))
{
mo->type = MO_USE;
- mo->u.loc = var_lowpart (mode_for_reg_attrs (*loc), *loc);
+ mo->u.loc = var_lowpart (mode, *loc);
}
else
{
@@ -1807,15 +1833,14 @@ add_uses (rtx *loc, void *insn)
mo->insn = (rtx) insn;
}
else if (MEM_P (*loc)
- && MEM_EXPR (*loc)
- && track_expr_p (MEM_EXPR (*loc))
- && offset_valid_for_tracked_p (INT_MEM_OFFSET (*loc)))
+ && track_loc_p (*loc, MEM_EXPR (*loc), INT_MEM_OFFSET (*loc),
+ false, &mode, NULL))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
mo->type = MO_USE;
- mo->u.loc = *loc;
+ mo->u.loc = var_lowpart (mode, *loc);
mo->insn = (rtx) insn;
}
@@ -1837,22 +1862,22 @@ add_uses_1 (rtx *x, void *insn)
static void
add_stores (rtx loc, const_rtx expr, void *insn)
{
+ enum machine_mode mode;
+
if (REG_P (loc))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
if (GET_CODE (expr) == CLOBBER
- || !(REG_EXPR (loc)
- && track_expr_p (REG_EXPR (loc))
- && offset_valid_for_tracked_p (REG_OFFSET (loc))))
+ || !track_loc_p (loc, REG_EXPR (loc), REG_OFFSET (loc),
+ true, &mode, NULL))
{
mo->type = MO_CLOBBER;
mo->u.loc = loc;
}
else
{
- enum machine_mode mode = mode_for_reg_attrs (loc);
rtx src = NULL;
if (GET_CODE (expr) == SET && SET_DEST (expr) == loc)
@@ -1878,9 +1903,8 @@ add_stores (rtx loc, const_rtx expr, voi
mo->insn = (rtx) insn;
}
else if (MEM_P (loc)
- && MEM_EXPR (loc)
- && track_expr_p (MEM_EXPR (loc))
- && offset_valid_for_tracked_p (INT_MEM_OFFSET (loc)))
+ && track_loc_p (loc, MEM_EXPR (loc), INT_MEM_OFFSET (loc),
+ false, &mode, NULL))
{
basic_block bb = BLOCK_FOR_INSN ((rtx) insn);
micro_operation *mo = VTI (bb)->mos + VTI (bb)->n_mos++;
@@ -1888,14 +1912,15 @@ add_stores (rtx loc, const_rtx expr, voi
if (GET_CODE (expr) == CLOBBER)
{
mo->type = MO_CLOBBER;
- mo->u.loc = loc;
+ mo->u.loc = var_lowpart (mode, loc);
}
else
{
rtx src = NULL;
if (GET_CODE (expr) == SET && SET_DEST (expr) == loc)
- src = var_lowpart (GET_MODE (loc), SET_SRC (expr));
+ src = var_lowpart (mode, SET_SRC (expr));
+ loc = var_lowpart (mode, loc);
if (src == NULL)
{
@@ -1904,6 +1929,8 @@ add_stores (rtx loc, const_rtx expr, voi
}
else
{
+ if (SET_SRC (expr) != src)
+ expr = gen_rtx_SET (VOIDmode, loc, src);
if (same_variable_part_p (SET_SRC (expr),
MEM_EXPR (loc),
INT_MEM_OFFSET (loc)))
@@ -3115,6 +3142,7 @@ vt_add_function_parameters (void)
rtx decl_rtl = DECL_RTL_IF_SET (parm);
rtx incoming = DECL_INCOMING_RTL (parm);
tree decl;
+ enum machine_mode mode;
HOST_WIDE_INT offset;
dataflow_set *out;
@@ -3131,18 +3159,26 @@ vt_add_function_parameters (void)
continue;
if (!vt_get_decl_and_offset (incoming, &decl, &offset))
- if (!vt_get_decl_and_offset (decl_rtl, &decl, &offset))
- continue;
+ {
+ if (!vt_get_decl_and_offset (decl_rtl, &decl, &offset))
+ continue;
+ offset += byte_lowpart_offset (GET_MODE (incoming),
+ GET_MODE (decl_rtl));
+ }
if (!decl)
continue;
gcc_assert (parm == decl);
+ if (!track_loc_p (incoming, parm, offset, false, &mode, &offset))
+ continue;
+
out = &VTI (ENTRY_BLOCK_PTR)->out;
if (REG_P (incoming))
{
+ incoming = var_lowpart (mode, incoming);
gcc_assert (REGNO (incoming) < FIRST_PSEUDO_REGISTER);
attrs_list_insert (&out->regs[REGNO (incoming)],
parm, offset, incoming);
@@ -3150,8 +3186,11 @@ vt_add_function_parameters (void)
NULL);
}
else if (MEM_P (incoming))
- set_variable_part (out, incoming, parm, offset, VAR_INIT_STATUS_INITIALIZED,
- NULL);
+ {
+ incoming = var_lowpart (mode, incoming);
+ set_variable_part (out, incoming, parm, offset,
+ VAR_INIT_STATUS_INITIALIZED, NULL);
+ }
}
}
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: inter.diff --]
[-- Type: text/x-diff, Size: 6332 bytes --]
diff -u gcc/rtl.h gcc/rtl.h
--- gcc/rtl.h (working copy)
+++ gcc/rtl.h 2007-12-18 07:48:07.000000000 +0000
@@ -149,7 +149,11 @@
} mem_attrs;
/* Structure used to describe the attributes of a REG in similar way as
- mem_attrs does for MEM above. */
+ mem_attrs does for MEM above. Note that the OFFSET field is calculated
+ in the same way as for mem_attrs, rather than in the same way as a
+ SUBREG_BYTE. For example, if a big-endian target stores a byte
+ object in the low part of a 4-byte register, the OFFSET field
+ will be -3 rather than 0. */
typedef struct reg_attrs GTY(())
{
diff -u gcc/emit-rtl.c gcc/emit-rtl.c
--- gcc/emit-rtl.c (working copy)
+++ gcc/emit-rtl.c 2007-12-18 09:25:48.000000000 +0000
@@ -837,10 +837,11 @@
return rt_val;
}
\f
-/* Return the byte offset of an OUTER_MODE value from an INNER_MODE value,
- given that the former is a lowpart of the latter. It may be a paradoxical
- lowpart, in which case the offset might be negative on big-endian
- targets. */
+/* Return the number of bytes between the start of an OUTER_MODE
+ in-memory value and the start of an INNER_MODE in-memory value,
+ given that the former is a lowpart of the latter. It may be a
+ paradoxical lowpart, in which case the offset will be negative
+ on big-endian targets. */
int
byte_lowpart_offset (enum machine_mode outer_mode,
@@ -906,8 +907,8 @@
return val;
}
-/* Update NEW with the same attributes as REG, but offsetted by OFFSET.
- Do the big endian correction if needed. */
+/* Update NEW with the same attributes as REG, but with OFFSET added
+ to the REG_OFFSET. */
static void
update_reg_offset (rtx new, rtx reg, int offset)
@@ -916,8 +917,8 @@
REG_OFFSET (reg) + offset);
}
-/* Generate a register with same attributes as REG, but offsetted by
- OFFSET. */
+/* Generate a register with same attributes as REG, but with OFFSET
+ added to the REG_OFFSET. */
rtx
gen_rtx_REG_offset (rtx reg, enum machine_mode mode, unsigned int regno,
@@ -930,7 +931,7 @@
}
/* Generate a new pseudo-register with the same attributes as REG, but
- offsetted by OFFSET. */
+ with OFFSET added to the REG_OFFSET. */
rtx
gen_reg_rtx_offset (rtx reg, enum machine_mode mode, int offset)
@@ -1239,8 +1240,7 @@
subreg_highpart_offset (outermode, innermode));
}
-/* Return offset in bytes to get OUTERMODE low part
- of the value in mode INNERMODE stored in memory in target format. */
+/* Return the SUBREG_BYTE for an OUTERMODE lowpart of an INNERMODE value. */
unsigned int
subreg_lowpart_offset (enum machine_mode outermode, enum machine_mode innermode)
diff -u gcc/reload.c gcc/reload.c
--- gcc/reload.c (working copy)
+++ gcc/reload.c 2007-12-18 07:48:07.000000000 +0000
@@ -6025,8 +6025,8 @@
XEXP (tem, 0) = plus_constant (XEXP (tem, 0), offset);
PUT_MODE (tem, GET_MODE (x));
- if (MEM_ATTRS (tem))
- set_mem_offset (tem, MEM_OFFSET (tem) + offset);
+ if (MEM_OFFSET (tem))
+ set_mem_offset (tem, plus_constant (MEM_OFFSET (tem), offset));
/* If this was a paradoxical subreg that we replaced, the
resulting memory must be sufficiently aligned to allow
diff -u gcc/var-tracking.c gcc/var-tracking.c
--- gcc/var-tracking.c (working copy)
+++ gcc/var-tracking.c 2007-12-18 07:48:07.000000000 +0000
@@ -1645,16 +1645,6 @@
return 1;
}
-/* Return true if OFFSET is a valid offset for a register or memory
- access we want to track. This is used to reject out-of-bounds
- accesses that can cause assertions to fail later. */
-
-static inline bool
-offset_valid_for_tracked_p (HOST_WIDE_INT offset)
-{
- return (offset >= 0) && (offset < MAX_VAR_PARTS);
-}
-
/* Determine whether a given LOC refers to the same variable part as
EXPR+OFFSET. */
@@ -1740,7 +1730,7 @@
offset = 0;
}
- if (!offset_valid_for_tracked_p (offset))
+ if (offset < 0 || offset >= MAX_VAR_PARTS)
return false;
if (mode_out)
only in patch2:
unchanged:
--- gcc/rtl.def 2007-12-18 07:39:35.000000000 +0000
+++ gcc/rtl.def 2007-12-18 07:48:07.000000000 +0000
@@ -371,14 +371,8 @@ DEF_RTL_EXPR(REG, "reg", "i00", RTX_OBJ)
marked as having one operand so it can be turned into a REG. */
DEF_RTL_EXPR(SCRATCH, "scratch", "0", RTX_OBJ)
-/* One word of a multi-word value.
- The first operand is the complete value; the second says which word.
- The WORDS_BIG_ENDIAN flag controls whether word number 0
- (as numbered in a SUBREG) is the most or least significant word.
-
- This is also used to refer to a value in a different machine mode.
- For example, it can be used to refer to a SImode value as if it were
- Qimode, or vice versa. Then the word number is always 0. */
+/* A reference to a part of another value. The first operand is the
+ complete value and the second is the byte offset of the selected part. */
DEF_RTL_EXPR(SUBREG, "subreg", "ei", RTX_EXTRA)
/* This one-argument rtx is used for move instructions
only in patch2:
unchanged:
--- gcc/final.c 2007-12-18 07:39:35.000000000 +0000
+++ gcc/final.c 2007-12-18 07:48:07.000000000 +0000
@@ -2763,8 +2763,15 @@ alter_subreg (rtx *xp)
else if (REG_P (y))
{
/* Simplify_subreg can't handle some REG cases, but we have to. */
- unsigned int regno = subreg_regno (x);
- *xp = gen_rtx_REG_offset (y, GET_MODE (x), regno, SUBREG_BYTE (x));
+ unsigned int regno;
+ HOST_WIDE_INT offset;
+
+ regno = subreg_regno (x);
+ if (subreg_lowpart_p (x))
+ offset = byte_lowpart_offset (GET_MODE (x), GET_MODE (y));
+ else
+ offset = SUBREG_BYTE (x);
+ *xp = gen_rtx_REG_offset (y, GET_MODE (x), regno, offset);
}
}
only in patch2:
unchanged:
--- gcc/config/ia64/ia64.c 2007-12-18 09:26:01.000000000 +0000
+++ gcc/config/ia64/ia64.c 2007-12-18 09:26:22.000000000 +0000
@@ -796,7 +796,8 @@ ia64_expand_load_address (rtx dest, rtx
computation below are also more natural to compute as 64-bit quantities.
If we've been given an SImode destination register, change it. */
if (GET_MODE (dest) != Pmode)
- dest = gen_rtx_REG_offset (dest, Pmode, REGNO (dest), 0);
+ dest = gen_rtx_REG_offset (dest, Pmode, REGNO (dest),
+ byte_lowpart_offset (Pmode, GET_MODE (dest)));
if (TARGET_NO_PIC)
return false;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fix PR rtl-optimization/33822
2007-12-18 10:47 ` Richard Sandiford
@ 2007-12-18 18:57 ` Eric Botcazou
2007-12-19 10:44 ` Richard Sandiford
0 siblings, 1 reply; 10+ messages in thread
From: Eric Botcazou @ 2007-12-18 18:57 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> I think rtl.texi already describes the SUBREG_BYTE calculation in enough
> detail, but I noticed that rtl.def was years out of date. I think that
> shows we should have detailed documentation in one place only (rtl.texi),
> so I simplified the rtl.def version.
Interesting left-overs of an old era. :-)
> I've made the comments for update_reg_offset, gen_rtx_REG_offset
> and gen_reg_rtx_offset say "with OFFSET added to the REG_OFFSET"
> rather than "offsetted by OFFSET". I think this, in combination
> with the reg_attrs comment, makes the type of offset more explicit.
Agreed.
> * rtl.def (SUBREG): Update comments.
> * rtl.h (reg_attrs): Be explicit about the type of offset used.
> (set_reg_attrs_from_mem): Rename to...
> (set_reg_attrs_from_value): ...this.
> (adjust_reg_mode, byte_lowpart_offset): Declare.
> * emit-rtl.c (byte_lowpart_offset): New function.
> (update_reg_offset): Remove special offset handling for big-endian
> targets.
> (gen_rtx_REG_offset, gen_reg_rtx_offset): Explicitly say that the
> offset parameter is added to REG_OFFSET.
> (adjust_reg_mode): New function.
> (set_reg_attrs_for_mem): Rename to...
> (set_reg_attrs_for_value): ...this and generalize to all values.
> If the register is a lowpart of the value, adjust the offset
> accordingly.
> (set_reg_attrs_for_parm): Update after the above renaming.
> (set_reg_attrs_for_decl_rtl): New function, split out from
> set_decl_incoming_rtl. Set the offset of plain REGs to the
> offset of the REG's mode from the decl's. Assert that all
> subregs are lowparts and handle their inner registers in the
> same way as plain REGs.
> (set_decl_rtl, set_incoming_decl_rtl): Use reg_attrs_for_decl_rtl.
> (subreg_lowpart_offset): Explicitly say that the returned offset
> is a SUBREG_BYTE.
> * combine.c (do_SUBST_MODE, try_combine, undo_all): Use adjust_reg_mode
> instead of PUT_MODE.
> * final.c (alter_subreg): Fix/update argument to gen_rtx_REG_offset.
> * config/ia64/ia64.c (ia64_expand_load_address): Likewise.
> * regclass.c (reg_scan_mark_refs): Use set_reg_attrs_from_value.
> * reload.c (find_reloads_subreg_address): Call set_mem_offset
> when offseting a MEM.
> * var-tracking.c (offset_valid_for_tracked_p): Delete.
> (mode_for_reg_attrs): Replace with...
> (track_loc_p): ...this new function. Return the mode and offset
> to the caller, checking that the latter is valid. If the rtx is
> a paradoxical lowpart of the decl, use the decl's mode instead.
> Do the same when storing to a register that contains the entire decl.
> (var_lowpart): Use byte_lowpart_offset rather than
> subreg_lowpart_offset when adjusting the offset attribute.
> (count_uses, add_uses, add_stores): Use track_reg_p instead of
> REG_EXPR, MEM_EXPR, REG_OFFSET, INT_MEM_OFFSET, track_expr_p,
> offset_valid_for_tracked_p and mode_for_reg_attrs. Generate
> lowparts for MEMs as well as REGs.
> (vt_add_function_parameters): When obtaining the information from
> the decl_rtl, adjust the offset to match incoming. Use track_loc_p
> and var_lowpart.
OK, thanks.
Btw, could we do something clever for the many similar calculations in
alter_subreg and simplify_subreg (and maybe elsewhere):
/* The SUBREG_BYTE represents offset, as if the value were stored
in memory. Irritating exception is paradoxical subreg, where
we define SUBREG_BYTE to be 0. On big endian machines, this
value should be negative. For a moment, undo this exception. */
if (byte == 0 && GET_MODE_SIZE (innermode) < GET_MODE_SIZE (outermode))
{
int difference = (GET_MODE_SIZE (innermode) - GET_MODE_SIZE (outermode));
if (WORDS_BIG_ENDIAN)
final_offset += (difference / UNITS_PER_WORD) * UNITS_PER_WORD;
if (BYTES_BIG_ENDIAN)
final_offset += difference % UNITS_PER_WORD;
--
Eric Botcazou
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Fix PR rtl-optimization/33822
2007-12-18 18:57 ` Eric Botcazou
@ 2007-12-19 10:44 ` Richard Sandiford
0 siblings, 0 replies; 10+ messages in thread
From: Richard Sandiford @ 2007-12-19 10:44 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Thanks for the review, now applied.
Eric Botcazou <ebotcazou@libertysurf.fr> writes:
> Btw, could we do something clever for the many similar calculations in
> alter_subreg and simplify_subreg (and maybe elsewhere):
>
> /* The SUBREG_BYTE represents offset, as if the value were stored
> in memory. Irritating exception is paradoxical subreg, where
> we define SUBREG_BYTE to be 0. On big endian machines, this
> value should be negative. For a moment, undo this exception. */
> if (byte == 0 && GET_MODE_SIZE (innermode) < GET_MODE_SIZE (outermode))
> {
> int difference = (GET_MODE_SIZE (innermode) - GET_MODE_SIZE (outermode));
> if (WORDS_BIG_ENDIAN)
> final_offset += (difference / UNITS_PER_WORD) * UNITS_PER_WORD;
> if (BYTES_BIG_ENDIAN)
> final_offset += difference % UNITS_PER_WORD;
TBH, I don't think the alter_subreg code should exist at all.
It implies that simplify_subreg is being too strict or that
reload is being too lax.
I'd rather get rid of the alter_subreg code (definitely not 4.3
material!) and fix any fallout. Introducing a new function for the
above smacks too much of simplify_subreg_this_time_i_really_mean_it.
Richard
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-12-19 10:00 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-11-07 20:49 Fix PR rtl-optimization/33822 Eric Botcazou
2007-11-09 22:14 ` Richard Sandiford
2007-11-12 15:39 ` Eric Botcazou
2007-11-15 12:31 ` Richard Sandiford
2007-11-15 12:44 ` RFA: Fix tracking of pass-by-reference parameters Richard Sandiford
2007-12-14 15:38 ` Eric Botcazou
2007-12-16 12:44 ` Fix PR rtl-optimization/33822 Eric Botcazou
2007-12-18 10:47 ` Richard Sandiford
2007-12-18 18:57 ` Eric Botcazou
2007-12-19 10:44 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).