On Wed, Nov 27, 2013 at 1:29 PM, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
> Hi,
>
> ping...
>
> this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html
>
> Note: it does, as it is, _not_ depend on the keep_aligning patch.
> And it would fix some really nasty wrong code generation issues.
I'll come back to all these patches once the late features have all
hit stage3 (hopefully next week).
We have some time left to fix this and related bugs.
Thanks,
Richard.
>
> Thanks
> Bernd.
>
>> Date: Tue, 19 Nov 2013 15:39:39 +0100
>>
>> Hello,
>>
>>
>> this is a minor update to my previous version of this patch, (using a
>> boolean expand_reference,
>> instead of adding a new expand_modifier enum value):
>>
>> I forgot to pass down the expand_reference value at the second expand_expr
>> call inside the
>> case VIEW_CONVERT_EXPR. Sorry for the inconvenience.
>>
>>
>>
>> @@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
>> }
>>
>> if (!op0)
>> - op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
>> + op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
>> + NULL, expand_reference);
>>
>> /* If the input and output modes are both the same, we are done. */
>> if (mode == GET_MODE (op0))
>>
>>
>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
>>
>> Ok for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>>> Date: Thu, 7 Nov 2013 13:58:55 +0100
>>>
>>> oops - this time with attachments...
>>>
>>>
>>>> Hi,
>>>>
>>>> On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
>>>>>
>>>>> On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
>>>>> <bernd.edlin...@hotmail.de> wrote:
>>>>>> Hi,
>>>>>>
>>>>>>> Eh ... even
>>>>>>>
>>>>>>> register struct { int i; int a[0]; } asm ("ebx");
>>>>>>>
>>>>>>> works. Also with int a[1] but not with a[2]. So just handling trailing
>>>>>>> arrays makes this case regress as well.
>>>>>>>
>>>>>>> Now I'd call this use questionable ... but we've likely supported that
>>>>>>> for decades and cannot change that now.
>>>>>>>
>>>>>>> Back to fixing everything in expand.
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>
>>>>>> Ok, finally you asked for it.
>>>>>>
>>>>>> Here is my previous version of that patch again.
>>>>>>
>>>>>> I have now added a new value "EXPAND_REFERENCE" to the expand_modifier
>>>>>> enumeration. It is almost like EXPAND_MEMORY but it does not interfere
>>>>>> with
>>>>>> constant values.
>>>>>>
>>>>>> I have done the same modification to VIEW_CONVERT_EXPR too, because
>>>>>> this is a possible inner reference, itself. It is however inherently
>>>>>> hard to
>>>>>> test around this code.
>>>>>>
>>>>>> To understand this patch it is good to know what type of object the
>>>>>> return value "tem" of get_inner_reference can be.
>>>>>>
>>>>>> From the program logic at get_inner_reference it is clear that the
>>>>>> return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
>>>>>> ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
>>>>>> be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
>>>>>> further restricted because exp is gimplified.
>>>>>>
>>>>>> Usually the result will be a MEM_REF or a SSA_NAME of the memory where
>>>>>> the structure is to be found.
>>>>>>
>>>>>> When you look at where EXPAND_MEMORY is handled you see it is
>>>>>> special-cased
>>>>>> in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
>>>>>> ARRAY_RANGE_REF.
>>>>>>
>>>>>> At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
>>>>>> same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
>>>>>> If it is an unaligned memory, we just return the unaligned reference.
>>>>>>
>>>>>> This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test
>>>>>> case,
>>>>>> because it is only a problem for STRICT_ALIGNMENT targets, and even
>>>>>> there it will
>>>>>> certainly be really hard to find test cases that exercise this code.
>>>>>>
>>>>>> In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
>>>>>> we do not have to touch the handling of the outer modifier. However we
>>>>>> pass
>>>>>> EXPAND_REFERENCE to the inner object, which should not be a recursive
>>>>>> use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
>>>>>>
>>>>>> TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
>>>>>> EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
>>>>>>
>>>>>>
>>>>>> Boot-strapped and regression-tested on x86_64-linux-gnu
>>>>>> OK for trunk?
>>>>>
>>>>> You point to a weak spot in expansion - that it handles creating
>>>>> the base MEM to offset with handled components by recursing
>>>>> into the case that handles bare MEM_REFs. This makes the
>>>>> bare MEM_REF handling code somewhat awkward (it's the
>>>>> one to assign mem-attrs which are later adjusted for example).
>>>>>
>>>>> Maybe a better appropach than adding yet another expand
>>>>> modifier would be to split out the "base MEM" expansion part
>>>>> out of the bare MEM_REF handling code so we can call that
>>>>> instead of recursing.
>>>>>
>>>>> In this light - instead of a new expand modifier don't you want
>>>>> an actual flag that specifies we are coming from a call that
>>>>> wants to expand a base? That is, allow EXPAND_SUM
>>>>> but with the recursion flag set?
>>>>>
>>>>
>>>> I think you are right. After some thought, I start to like that idea.
>>>>
>>>> This way we have at least much more flexibility, how to handle the inner
>>>> references correctly, and if I change only the interfaces of
>>>> expand_expr_real/real_1
>>>> that will not be used at too many places, either.
>>>>
>>>>> Finally I think the recursion into the VIEW_CONVERT_EXPR case
>>>>> is only there because of the keep_aligning flag of get_inner_reference
>>>>> which should be obsolete now that we properly handle its effects
>>>>> in get_object_alignment. So you wouldn't need to adjust this path
>>>>> if we finally can get rid of that.
>>>>>
>>>>
>>>> I proposed a patch for that, but did not hear from you:
>>>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html
>>>>
>>>> nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner
>>>> reference, the code there should be kept in sync with the normal_inner_ref,
>>>> and MEM_REF, IMHO.
>>>>
>>>> Attached, my updated patch for PR57748, Part 2.
>>>>
>>>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
>>>>
>>>> Ok for trunk?
>>>>
>>>>
>>>> Thanks
>>>> Bernd.
>>>>
>>>>> What do others think?
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> Thanks
>>>>>> Bernd.
