This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Fix PR rtl-optimization/33822
- From: Eric Botcazou <ebotcazou at libertysurf dot fr>
- To: Richard Sandiford <rsandifo at nildram dot co dot uk>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Sun, 16 Dec 2007 13:39:19 +0100
- Subject: Re: Fix PR rtl-optimization/33822
- References: <200711072150.47455.ebotcazou@libertysurf.fr> <200711121559.01676.ebotcazou@libertysurf.fr> <87mytfu8cc.fsf@firetop.home>
> 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