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

[H.J. Lu]

On Wed, Feb 9, 2011 at 4:10 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Feb 10, 2011 at 12:39 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Feb 10, 2011 at 12:37 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Wed, Feb 9, 2011 at 3:30 PM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Thu, Feb 10, 2011 at 12:05 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Wed, Feb 9, 2011 at 1:35 PM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Wed, Feb 9, 2011 at 10:28 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> On Wed, Feb 9, 2011 at 1:22 PM, Richard Guenther
>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>> On Wed, Feb 9, 2011 at 10: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?
>>>>>>>>
>>>>>>>> From the default hook implementation that indeed seems to be the case.
>>>>>>>> Using ptr_mode might be undesirable as then the generated RTL will
>>>>>>>> probably never match complex addressing modes.  Thus the component
>>>>>>>> and address types used for TARGET_MEM_REF are not suitable for
>>>>>>>> a Pmode != ptr_mode target and would have to use a pointer type
>>>>>>>> of Pmode mode (which we don't have, and frankly introducing such a
>>>>>>>> 2nd pointer mode on trees might have non-trivial effects).  Eventually
>>>>>>>> just forcing the offset components of TARGET_MEM_REF to Pmode
>>>>>>>> size would also be a possibility (though you will run into the issue that
>>>>>>>> we loose any signedness of pointer offsets by casting them to sizetype).
>>>>>>>
>>>>>>> The offset components have to be signed while ptr_mode to Pmode
>>>>>>> may be zero extended.
>>>>>>
>>>>>> The sign of the offset components does not matter if they have the
>>>>>> same width as the resulting pointer.  Or do you say we have
>>>>>> a %eax + %ebx addressing mode that zero-extends eax and sign-extends
>>>>>> ebx?  Or that we can use a 32bit offset register that will be sign-extended?
>>>>>
>>>>> x32 uses %rax + %rbx * scale. RAX is zero-extended and
>>>>> RBX is sign-extended.
>>>>
>>>> how can %rax or %rbx be extended further?  Those are 64bit registers.
>>>
>>> RAX is zero-extended from EAX and RBX is sign-extended from EBX,
>>
>> By separate instructions?
>
> By combined brain-power we dug out the last sentence of 3.3.7
> of the ia32 basic arch manual which says the address is zero-extended
> from 32bit after computing it.  So there is no problem?
>

In x32 mode, we use (%rax,%rbx,4), not (%eax,%ebx,4).  When we generate
(%rax,%rbx,4) in x32 mode, we have to put sign-extended value in RBX.

-- 
H.J.