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. 2013-11-07 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR middle-end/57748
* expr.h (expand_expr_real, expand_expr_real_1): Add new parameter
expand_reference.
(expand_expr, expand_normal): Adjust.
* expr.c (expand_expr_real, expand_expr_real_1): Add new parameter
expand_reference. Use expand_reference to expand inner references.
(store_expr): Adjust.
* cfgexpand.c (expand_call_stmt): Adjust.
testsuite:
2013-11-07 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR middle-end/57748
* gcc.dg/torture/pr57748-3.c: New test.
* gcc.dg/torture/pr57748-4.c: New test.
patch-pr57748-2.diff
Description: Binary data
