Re: PATCH: PR middle-end/47383: ivopts miscompiles Pmode != ptr_mode

[Richard Guenther <richard dot guenther at gmail dot com>]

On Sat, Feb 12, 2011 at 6:27 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Feb 9, 2011 at 1:17 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Feb 9, 2011 at 9:45 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Wed, Feb 9, 2011 at 12:17 PM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Feb 9, 2011 at 6:17 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Wed, Feb 9, 2011 at 2:04 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Wed, Feb 9, 2011 at 12:08 AM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> tree-ssa-loop-ivopts.c has
>>>>>>>
>>>>>>> ---
>>>>>>> /* Returns variant of TYPE that can be used as base for different uses.
>>>>>>> ? We return unsigned type with the same precision, which avoids problems
>>>>>>> ? with overflows. ?*/
>>>>>>>
>>>>>>> static tree
>>>>>>> generic_type_for (tree type)
>>>>>>> {
>>>>>>> ?if (POINTER_TYPE_P (type))
>>>>>>> ? ?return unsigned_type_for (type);
>>>>>>>
>>>>>>> ?if (TYPE_UNSIGNED (type))
>>>>>>> ? ?return type;
>>>>>>>
>>>>>>> ?return unsigned_type_for (type);
>>>>>>> }
>>>>>>> ---
>>>>>>>
>>>>>>> It converts negative step to unsigned type with the same precision.
>>>>>>> It doesn't work when Pmode != ptr_mode. ?This patch disables it.
>>>>>>> OK for trunk in stage 1?
>>>>>>
>>>>>> This does not look correct. ?It rather sounds you are expanding
>>>>>> TARGET_MEM_REFs the wrong way - the address computed by
>>>>>> it is supposed to be calculated in ptr_mode and only the final
>>>>>> result extended to Pmode.
>>>>>>
>>>>>
>>>>>
>>>>> TARGET_MEM_REF is OK. The problem is ivopts passes
>>>>> the negative offset to TARGET_MEM_REF as unsigned and
>>>>> expects offset will wrap.
>>>>
>>>> But they will wrap, as arithmetic is carried out in a type with
>>>> the same precision (that of ptr_mode).
>>>>
>>>>> ?It only works when Pmode ==
>>>>> ptr_mode. ?I am checking in this patch into x32 branch to
>>>>> avoid such cases.
>>>>
>>>> I think you are wrong.
>>>>
>>>
>>> If you have a patch, I can give a try.
>>
>> I'd have to debug it and I don't have a target that shows the failure, but
>> just looking around I assume that in
>>
>> rtx
>> addr_for_mem_ref (struct mem_address *addr, addr_space_t as,
>> ? ? ? ? ? ? ? ? ?bool really_expand)
>> {
>> ?enum machine_mode address_mode = targetm.addr_space.address_mode (as);
>>
>> address_mode will be Pmode - that would be already wrong. ?We need to
>> use ptr_mode here and at the end of the function convert the
>> generated address to Pmode appropriately. ?Can you verify that theory?
>>
>
> This patch works for me. ?However, if I add
>
> diff --git a/gcc/tree-ssa-address.c b/gcc/tree-ssa-address.c
> index 7b7e169..d9d906e 100644
> --- a/gcc/tree-ssa-address.c
> +++ b/gcc/tree-ssa-address.c
> @@ -242,6 +242,9 @@ addr_for_mem_ref (struct mem_address *addr, addr_space_t as,
> ? ? ? if (off)
> ? *templ->off_p = off;
>
> + ? ? ?if (targetm.addr_space.address_mode (as) != ptr_mode)
> + ?templ->ref = convert_to_mode (targetm.addr_space.address_mode (as),
> + ? ? ? ? ? ? ? ? templ->ref, 1);
> ? ? ? return templ->ref;
> ? ? }
>
> to convert MEM_REF from ptr_mode to Pmode, I get
>
> /export/build/gnu/gcc-x32/build-x86_64-linux/gcc/xgcc
> -B/export/build/gnu/gcc-x32/build-x86_64-linux/gcc/ -S -o x.s -mx32
> -O2 -g -fPIC ? ?x.i
> x.i: In function \u2018foo\u2019:
> x.i:21:1: error: insn outside basic block
> (insn 88 0 113 (parallel [
> ? ? ? ? ? ?(set (reg:SI 143)
> ? ? ? ? ? ? ? ?(ashift:SI (reg:SI 60)
> ? ? ? ? ? ? ? ? ? ?(const_int 3 [0x3])))
> ? ? ? ? ? ?(clobber (reg:CC 17 flags))
> ? ? ? ?]) -1
> ? ? (nil))
> x.i:21:1: internal compiler error: in rtl_verify_flow_info, at cfgrtl.c:2218
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
> make: *** [x.s] Error 1
>
> Do I really need to convert MEM_REF from ptr_mode to Pmode?

Only if you do not plan to support 32bit addresses in the backend.
The above is probably
because the caller context from where that function is called isn't
prepared to have
instructions emitted (maybe it's called from the cost analysis pieces as well?)

Richard.

> Thanks.
>
> --
> H.J.
> ---
> diff --git a/gcc/tree-ssa-address.c b/gcc/tree-ssa-address.c
> index a9ca835..7b7e169 100644
> --- a/gcc/tree-ssa-address.c
> +++ b/gcc/tree-ssa-address.c
> @@ -188,12 +188,12 @@ rtx
> ?addr_for_mem_ref (struct mem_address *addr, addr_space_t as,
> ? ? ? ? ? ? ? ? ?bool really_expand)
> ?{
> - ?enum machine_mode address_mode = targetm.addr_space.address_mode (as);
> ? rtx address, sym, bse, idx, st, off;
> ? struct mem_addr_template *templ;
>
> ? if (addr->step && !integer_onep (addr->step))
> - ? ?st = immed_double_int_const (tree_to_double_int (addr->step), address_mode)
> ;
> + ? ?st = immed_double_int_const (tree_to_double_int (addr->step),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?ptr_mode);
> ? else
> ? ? st = NULL_RTX;
>
> @@ -201,7 +201,7 @@ addr_for_mem_ref (struct mem_address *addr, addr_space_t as,
> ? ? off = immed_double_int_const
> ? ? ? ? ? ?(double_int_sext (tree_to_double_int (addr->offset),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TYPE_PRECISION (TREE_TYPE (addr->offset))),
> - ? ? ? ? ? ?address_mode);
> + ? ? ? ? ? ?ptr_mode);
> ? else
> ? ? off = NULL_RTX;
>
> @@ -220,16 +220,16 @@ addr_for_mem_ref (struct mem_address *addr, addr_space_t a
> s,
> ? ? ? if (!templ->ref)
> ? ? ? ?{
> ? ? ? ? ?sym = (addr->symbol ?
> - ? ? ? ? ? ? ? ?gen_rtx_SYMBOL_REF (address_mode, ggc_strdup ("test_symbol"))
> + ? ? ? ? ? ? ? ?gen_rtx_SYMBOL_REF (ptr_mode, ggc_strdup ("test_symbol"))
> ? ? ? ? ? ? ? ? : NULL_RTX);
> ? ? ? ? ?bse = (addr->base ?
> - ? ? ? ? ? ? ? ?gen_raw_REG (address_mode, LAST_VIRTUAL_REGISTER + 1)
> + ? ? ? ? ? ? ? ?gen_raw_REG (ptr_mode, LAST_VIRTUAL_REGISTER + 1)
> ? ? ? ? ? ? ? ? : NULL_RTX);
> ? ? ? ? ?idx = (addr->index ?
> - ? ? ? ? ? ? ? ?gen_raw_REG (address_mode, LAST_VIRTUAL_REGISTER + 2)
> + ? ? ? ? ? ? ? ?gen_raw_REG (ptr_mode, LAST_VIRTUAL_REGISTER + 2)
> ? ? ? ? ? ? ? ? : NULL_RTX);
>
> - ? ? ? ? gen_addr_rtx (address_mode, sym, bse, idx,
> + ? ? ? ? gen_addr_rtx (ptr_mode, sym, bse, idx,
> ? ? ? ? ? ? ? ? ? ? ? ?st? const0_rtx : NULL_RTX,
> ? ? ? ? ? ? ? ? ? ? ? ?off? const0_rtx : NULL_RTX,
> ? ? ? ? ? ? ? ? ? ? ? ?&templ->ref,
> @@ -247,16 +247,16 @@ addr_for_mem_ref (struct mem_address *addr, addr_space_t a
> s,
>
> ? /* Otherwise really expand the expressions. ?*/
> ? sym = (addr->symbol
> - ? ? ? ?? expand_expr (addr->symbol, NULL_RTX, address_mode, EXPAND_NORMAL)
> + ? ? ? ?? expand_expr (addr->symbol, NULL_RTX, ptr_mode, EXPAND_NORMAL)
> ? ? ? ? : NULL_RTX);
> ? bse = (addr->base
> - ? ? ? ?? expand_expr (addr->base, NULL_RTX, address_mode, EXPAND_NORMAL)
> + ? ? ? ?? expand_expr (addr->base, NULL_RTX, ptr_mode, EXPAND_NORMAL)
> ? ? ? ? : NULL_RTX);
> ? idx = (addr->index
> - ? ? ? ?? expand_expr (addr->index, NULL_RTX, address_mode, EXPAND_NORMAL)
> + ? ? ? ?? expand_expr (addr->index, NULL_RTX, ptr_mode, EXPAND_NORMAL)
> ? ? ? ? : NULL_RTX);
>
> - ?gen_addr_rtx (address_mode, sym, bse, idx, st, off, &address, NULL, NULL);
> + ?gen_addr_rtx (ptr_mode, sym, bse, idx, st, off, &address, NULL, NULL);
> ? return address;
> ?}
>