On Wed, Oct 16, 2013 at 7:53 PM, Bernd Schmidt <ber...@codesourcery.com> wrote:
> The sequence of events here can be summarized as "shrink-wrapping causes
> the i386 backend to do something that confuses alias analysis". The
> miscompilation is that two instructions are swapped by the scheduler
> when they shouldn't be, due to an incorrect reg_base_value.
>
> The miscompiled function has the following parts:
> * a loop which uses %rdi as an incoming argument register
> * following that, a decrement of the stack pointer (shrink-wrapped
> to this point rather than the start of the function)
> * the rest of the function which has one set of %rdi to
> (symbol_ref LC0).
>
> The argument register %rdi is dead by the point where we decrement the
> stack pointer. The i386 backend splits the sub into a sequence of two
> insns, a clobber of %rdi and a push of it (which register is chosen
> appears to be somewhat random).
>
> When called from the scheduler, init_alias_analysis incorrectly deduces
> that %rdi has a base value of (symbol_ref LC0). Although it tries to
> track which registers are argument registers that may hold a pointer,
> this information is lost when we encounter the clobber. The main part of
> the following patch is to modify that piece of code to also set reg_seen
> for argument registers when encountering a clobber.
>
> There are other problems in this area, one of which showed up while
> testing an earlier patch. i386/pr57003.c demonstrates that return values
> of FUNCTION_ARG_REGNO are not constant across the whole compilation;
> they are affected by the ms_abi attribute. This necessitates changing
> from a static_reg_base_value array to a per-function one. Once that is
> done, it's better to look at DECL_ARGUMENTS in a similar way to what
> combine.c does to identify the arguments of the specific function we're
> compiling rather than using the less specific FUNCTION_ARG_REGNO.
> Lastly, I modified a test for Pmode to use the valid_pointer_mode hook
> which should be a little more correct.
>
> Bootstrapped and tested on x86_64-linux. Ok?
+ if (!bitmap_bit_p (reg_seen, regno))
+ {
+ /* We have to make an exception here for an argument
+ register, in case it is used before the clobber. */
+ gcc_assert (new_reg_base_value[regno] == arg_base_value);
+ bitmap_set_bit (reg_seen, regno);
+ }
please use gcc_checking_assert. (bah, I hate that subtle difference
in sbitmap vs. bitmap not returning whether the bit changed in
the modifying operations ...)
+ /* We have to make an exception here for an argument
+ register, in case it is used before the clobber. */
can you elaborate here _why_ we have to make that exception?
Otherwise looks good to me (with my limited knowledge of this area).
Thanks,
Richard.
>
> Bernd
