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. 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.
