* [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)
@ 2019-09-03 7:02 Bernd Edlinger
2019-09-03 7:05 ` Jakub Jelinek
0 siblings, 1 reply; 8+ messages in thread
From: Bernd Edlinger @ 2019-09-03 7:02 UTC (permalink / raw)
To: gcc-patches, Richard Biener, Jeff Law
[-- Attachment #1: Type: text/plain, Size: 573 bytes --]
Hi,
this fixes two bugs, one is also a wrong-code bug that turned into an ICE due
to the middle-end sanitation (PR 91603/91612). The other is just an assertion
due to expand_expr_real_1 setting byte-aligned mem_attributes of a SSA_NAME
referring to a CONSTANT_P which is actually 16-byte aligned, this happens
after the we handle misaligned stuff, and therefore causes assertions when
that value is used later on, but this is no wrong-code.
Bootstrapped and reg-tested on x86_64-pc-linux-gnu and arm-linux-gnueabihf.
Is it OK for trunk?
Thanks
Bernd.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr91612.diff --]
[-- Type: text/x-patch; name="patch-pr91612.diff", Size: 3454 bytes --]
2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
PR middle-end/91603
PR middle-end/91612
PR middle-end/91613
* expr.c (expand_expr_real_1): decl_p_1): Refactor into...
(non_mem_decl_p): ...this.
(mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
(expand_assignment): Call mem_ref_referes_to_non_mem_p
unconditionally as before.
testsuite:
2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
PR middle-end/91603
* testsuite/gcc.target/arm/pr91603.c: New test.
Index: gcc/expr.c
===================================================================
--- gcc/expr.c (Revision 275320)
+++ gcc/expr.c (Arbeitskopie)
@@ -10062,7 +10062,43 @@ expand_expr_real_1 (tree exp, rtx target, machine_
{
if (exp && MEM_P (temp) && REG_P (XEXP (temp, 0)))
mark_reg_pointer (XEXP (temp, 0), DECL_ALIGN (exp));
+ }
+ else if (MEM_P (decl_rtl))
+ temp = decl_rtl;
+ if (temp != 0)
+ {
+ if (MEM_P (temp)
+ && modifier != EXPAND_WRITE
+ && modifier != EXPAND_MEMORY
+ && modifier != EXPAND_INITIALIZER
+ && modifier != EXPAND_CONST_ADDRESS
+ && modifier != EXPAND_SUM
+ && !inner_reference_p
+ && mode != BLKmode
+ && MEM_ALIGN (temp) < GET_MODE_ALIGNMENT (mode))
+ {
+ enum insn_code icode;
+
+ if ((icode = optab_handler (movmisalign_optab, mode))
+ != CODE_FOR_nothing)
+ {
+ class expand_operand ops[2];
+
+ /* We've already validated the memory, and we're creating a
+ new pseudo destination. The predicates really can't fail,
+ nor can the generator. */
+ create_output_operand (&ops[0], NULL_RTX, mode);
+ create_fixed_operand (&ops[1], temp);
+ expand_insn (icode, 2, ops);
+ temp = ops[0].value;
+ }
+ else if (targetm.slow_unaligned_access (mode, MEM_ALIGN (temp)))
+ temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
+ 0, unsignedp, NULL_RTX,
+ mode, mode, false, NULL);
+ }
+
return temp;
}
@@ -10974,9 +11010,10 @@ expand_expr_real_1 (tree exp, rtx target, machine_
op0 = copy_rtx (op0);
/* Don't set memory attributes if the base expression is
- SSA_NAME that got expanded as a MEM. In that case, we should
- just honor its original memory attributes. */
- if (TREE_CODE (tem) != SSA_NAME || !MEM_P (orig_op0))
+ SSA_NAME that got expanded as a MEM or a CONSTANT. In that case,
+ we should just honor its original memory attributes. */
+ if (!(TREE_CODE (tem) == SSA_NAME
+ && (MEM_P (orig_op0) || CONSTANT_P (orig_op0))))
set_mem_attributes (op0, exp, 0);
if (REG_P (XEXP (op0, 0)))
Index: gcc/testsuite/gcc.target/arm/pr91603.c
===================================================================
--- gcc/testsuite/gcc.target/arm/pr91603.c (Revision 0)
+++ gcc/testsuite/gcc.target/arm/pr91603.c (Arbeitskopie)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_neon_ok } */
+/* { dg-options "-O3" } */
+/* { dg-add-options arm_neon } */
+
+typedef __simd64_int32_t int32x2_t;
+typedef __attribute__((aligned (1))) int32x2_t unalignedvec;
+
+unalignedvec a = {11, 13};
+
+void foo(unalignedvec *);
+
+void test()
+{
+ unalignedvec x = a;
+ foo (&x);
+ a = x;
+}
+
+/* { dg-final { scan-assembler-times "vld1.32" 1 } } */
+/* { dg-final { scan-assembler-times "vst1.32" 1 } } */
+/* { dg-final { scan-assembler-times "vldr" 1 } } */
+/* { dg-final { scan-assembler-times "vstr" 1 } } */
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)
2019-09-03 7:02 [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613) Bernd Edlinger
@ 2019-09-03 7:05 ` Jakub Jelinek
2019-09-03 7:11 ` Bernd Edlinger
0 siblings, 1 reply; 8+ messages in thread
From: Jakub Jelinek @ 2019-09-03 7:05 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: gcc-patches, Richard Biener, Jeff Law
On Tue, Sep 03, 2019 at 07:02:53AM +0000, Bernd Edlinger wrote:
> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>
> PR middle-end/91603
> PR middle-end/91612
> PR middle-end/91613
> * expr.c (expand_expr_real_1): decl_p_1): Refactor into...
> (non_mem_decl_p): ...this.
> (mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
> (expand_assignment): Call mem_ref_referes_to_non_mem_p
> unconditionally as before.
Not a review, just questioning the ChangeLog entry.
What is the "decl_p_1): " in there? Also, the ChangeLog mentions many
functions, but the patch in reality just modifies expand_expr_real_1
and nothing else.
Jakub
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)
2019-09-03 7:05 ` Jakub Jelinek
@ 2019-09-03 7:11 ` Bernd Edlinger
2019-09-03 11:12 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Bernd Edlinger @ 2019-09-03 7:11 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, Richard Biener, Jeff Law
On 9/3/19 9:05 AM, Jakub Jelinek wrote:
> On Tue, Sep 03, 2019 at 07:02:53AM +0000, Bernd Edlinger wrote:
>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> PR middle-end/91603
>> PR middle-end/91612
>> PR middle-end/91613
>> * expr.c (expand_expr_real_1): decl_p_1): Refactor into...
>> (non_mem_decl_p): ...this.
>> (mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
>> (expand_assignment): Call mem_ref_referes_to_non_mem_p
>> unconditionally as before.
>
> Not a review, just questioning the ChangeLog entry.
> What is the "decl_p_1): " in there? Also, the ChangeLog mentions many
> functions, but the patch in reality just modifies expand_expr_real_1
> and nothing else.
>
Ah, sorry, this is of course wrong, (I forgot to complete the sentence,
and later forgot to check it again)....
This is what I actually wanted to say:
2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
PR middle-end/91603
PR middle-end/91612
PR middle-end/91613
* expr.c (expand_expr_real_1): Handle unaligned decl_rtl
and SSA_NAME referring to CONSTANT_P correctly.
testsuite:
2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
PR middle-end/91603
* testsuite/gcc.target/arm/pr91603.c: New test.
Thanks
Bernd.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)
2019-09-03 7:11 ` Bernd Edlinger
@ 2019-09-03 11:12 ` Richard Biener
2019-09-03 13:02 ` Bernd Edlinger
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2019-09-03 11:12 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: Jakub Jelinek, gcc-patches, Jeff Law
On Tue, 3 Sep 2019, Bernd Edlinger wrote:
> On 9/3/19 9:05 AM, Jakub Jelinek wrote:
> > On Tue, Sep 03, 2019 at 07:02:53AM +0000, Bernd Edlinger wrote:
> >> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
> >>
> >> PR middle-end/91603
> >> PR middle-end/91612
> >> PR middle-end/91613
> >> * expr.c (expand_expr_real_1): decl_p_1): Refactor into...
> >> (non_mem_decl_p): ...this.
> >> (mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
> >> (expand_assignment): Call mem_ref_referes_to_non_mem_p
> >> unconditionally as before.
> >
> > Not a review, just questioning the ChangeLog entry.
> > What is the "decl_p_1): " in there? Also, the ChangeLog mentions many
> > functions, but the patch in reality just modifies expand_expr_real_1
> > and nothing else.
> >
>
> Ah, sorry, this is of course wrong, (I forgot to complete the sentence,
> and later forgot to check it again)....
>
>
> This is what I actually wanted to say:
>
> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>
> PR middle-end/91603
> PR middle-end/91612
> PR middle-end/91613
> * expr.c (expand_expr_real_1): Handle unaligned decl_rtl
> and SSA_NAME referring to CONSTANT_P correctly.
>
> testsuite:
> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>
> PR middle-end/91603
> * testsuite/gcc.target/arm/pr91603.c: New test.
@@ -10062,7 +10062,43 @@ expand_expr_real_1 (tree exp, rtx target,
machine_
{
if (exp && MEM_P (temp) && REG_P (XEXP (temp, 0)))
mark_reg_pointer (XEXP (temp, 0), DECL_ALIGN (exp));
+ }
+ else if (MEM_P (decl_rtl))
+ temp = decl_rtl;
+ if (temp != 0)
+ {
+ if (MEM_P (temp)
+ && modifier != EXPAND_WRITE
+ && modifier != EXPAND_MEMORY
+ && modifier != EXPAND_INITIALIZER
+ && modifier != EXPAND_CONST_ADDRESS
+ && modifier != EXPAND_SUM
+ && !inner_reference_p
+ && mode != BLKmode
+ && MEM_ALIGN (temp) < GET_MODE_ALIGNMENT (mode))
So other places ([TARGET_]MEM_REF cases) use "just"
if (modifier != EXPAND_WRITE
&& modifier != EXPAND_MEMORY
&& !inner_reference_p
&& mode != BLKmode
&& align < GET_MODE_ALIGNMENT (mode))
I also wonder if you can split out all this common code to
a function (the actual unaligned expansion, that is) and call
it from those places (where the TARGET_MEM_REF case misses the
slow_unaligned_access case - presumably wanting to "assert"
that this doesn't happen.
/* If the target does not have special handling for unaligned
loads of mode then it can use regular moves for them. */
Richard.
+ {
+ enum insn_code icode;
+
+ if ((icode = optab_handler (movmisalign_optab, mode))
+ != CODE_FOR_nothing)
+ {
+ class expand_operand ops[2];
+
+ /* We've already validated the memory, and we're
creating
a
+ new pseudo destination. The predicates really can't
fail,
+ nor can the generator. */
+ create_output_operand (&ops[0], NULL_RTX, mode);
+ create_fixed_operand (&ops[1], temp);
+ expand_insn (icode, 2, ops);
+ temp = ops[0].value;
+ }
+ else if (targetm.slow_unaligned_access (mode, MEM_ALIGN
(temp)))
+ temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
+ 0, unsignedp, NULL_RTX,
+ mode, mode, false, NULL);
+ }
+
return temp;
}
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)
2019-09-03 11:12 ` Richard Biener
@ 2019-09-03 13:02 ` Bernd Edlinger
2019-09-03 13:12 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Bernd Edlinger @ 2019-09-03 13:02 UTC (permalink / raw)
To: Richard Biener; +Cc: Jakub Jelinek, gcc-patches, Jeff Law
On 9/3/19 1:12 PM, Richard Biener wrote:
> On Tue, 3 Sep 2019, Bernd Edlinger wrote:
>
>> On 9/3/19 9:05 AM, Jakub Jelinek wrote:
>>> On Tue, Sep 03, 2019 at 07:02:53AM +0000, Bernd Edlinger wrote:
>>>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>>
>>>> PR middle-end/91603
>>>> PR middle-end/91612
>>>> PR middle-end/91613
>>>> * expr.c (expand_expr_real_1): decl_p_1): Refactor into...
>>>> (non_mem_decl_p): ...this.
>>>> (mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
>>>> (expand_assignment): Call mem_ref_referes_to_non_mem_p
>>>> unconditionally as before.
>>>
>>> Not a review, just questioning the ChangeLog entry.
>>> What is the "decl_p_1): " in there? Also, the ChangeLog mentions many
>>> functions, but the patch in reality just modifies expand_expr_real_1
>>> and nothing else.
>>>
>>
>> Ah, sorry, this is of course wrong, (I forgot to complete the sentence,
>> and later forgot to check it again)....
>>
>>
>> This is what I actually wanted to say:
>>
>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> PR middle-end/91603
>> PR middle-end/91612
>> PR middle-end/91613
>> * expr.c (expand_expr_real_1): Handle unaligned decl_rtl
>> and SSA_NAME referring to CONSTANT_P correctly.
>>
>> testsuite:
>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>
>> PR middle-end/91603
>> * testsuite/gcc.target/arm/pr91603.c: New test.
>
> @@ -10062,7 +10062,43 @@ expand_expr_real_1 (tree exp, rtx target,
> machine_
> {
> if (exp && MEM_P (temp) && REG_P (XEXP (temp, 0)))
> mark_reg_pointer (XEXP (temp, 0), DECL_ALIGN (exp));
> + }
> + else if (MEM_P (decl_rtl))
> + temp = decl_rtl;
>
> + if (temp != 0)
> + {
> + if (MEM_P (temp)
> + && modifier != EXPAND_WRITE
> + && modifier != EXPAND_MEMORY
> + && modifier != EXPAND_INITIALIZER
> + && modifier != EXPAND_CONST_ADDRESS
> + && modifier != EXPAND_SUM
> + && !inner_reference_p
> + && mode != BLKmode
> + && MEM_ALIGN (temp) < GET_MODE_ALIGNMENT (mode))
>
> So other places ([TARGET_]MEM_REF cases) use "just"
>
Yes, interesting all of them do slightly different things.
I started with cloning the MEM_REF case, but it ran immediately
into issues with this assert here:
result = expand_expr (exp, target, tmode,
modifier == EXPAND_INITIALIZER
? EXPAND_INITIALIZER : EXPAND_CONST_ADDRESS);
/* If the DECL isn't in memory, then the DECL wasn't properly
marked TREE_ADDRESSABLE, which will be either a front-end
or a tree optimizer bug. */
gcc_assert (MEM_P (result));
result = XEXP (result, 0);
which implies that I need to add EXPAND_INITIALIZER and EXPAND_CONST_ADDRESS,
but since the code immediately above also has an exception of EXPAND_SUM:
else if (MEM_P (decl_rtl) && modifier != EXPAND_INITIALIZER)
{
if (alt_rtl)
*alt_rtl = decl_rtl;
decl_rtl = use_anchored_address (decl_rtl);
if (modifier != EXPAND_CONST_ADDRESS
&& modifier != EXPAND_SUM
I thought it I need to add also an exception for EXPAND_SUM.
Probably there is a reason why TARGET_MEM_REF does not need the
extract_bit_field stuff, when I read the comment here:
/* If the target does not have special handling for unaligned
loads of mode then it can use regular moves for them. */
&& ((icode = optab_handler (movmisalign_optab, mode))
!= CODE_FOR_nothing))
it is just, I don't really believe it.
> if (modifier != EXPAND_WRITE
> && modifier != EXPAND_MEMORY
> && !inner_reference_p
> && mode != BLKmode
> && align < GET_MODE_ALIGNMENT (mode))
>
> I also wonder if you can split out all this common code to
> a function (the actual unaligned expansion, that is) and call
> it from those places (where the TARGET_MEM_REF case misses the
> slow_unaligned_access case - presumably wanting to "assert"
> that this doesn't happen.
>
> /* If the target does not have special handling for unaligned
> loads of mode then it can use regular moves for them. */
>
Actually there is still a small difference to the MEM_REF expansion,
see the alt_rtl and the EXPAND_STACK_PARAM:
temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
0, TYPE_UNSIGNED (TREE_TYPE (exp)),
(modifier == EXPAND_STACK_PARM
? NULL_RTX : target),
mode, mode, false, alt_rtl);
TARGET_MEM_REF does not do extract_bit_field at all,
while I think ignoring target and alt_rtl in the DECL_P case is safe,
target, because it is at most a missed optimization, and
alt_rtl because it should already be handled above?
But if I pass target I cannot simply ignore alt_rtl any more?
Well, I could pass target and alt_rtl differently each time.
should I still try to factor that into a single function, it will have
around 7 parameters?
Bernd.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)
2019-09-03 13:02 ` Bernd Edlinger
@ 2019-09-03 13:12 ` Richard Biener
2019-09-03 13:13 ` Richard Biener
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2019-09-03 13:12 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: Jakub Jelinek, gcc-patches, Jeff Law
On Tue, 3 Sep 2019, Bernd Edlinger wrote:
> On 9/3/19 1:12 PM, Richard Biener wrote:
> > On Tue, 3 Sep 2019, Bernd Edlinger wrote:
> >
> >> On 9/3/19 9:05 AM, Jakub Jelinek wrote:
> >>> On Tue, Sep 03, 2019 at 07:02:53AM +0000, Bernd Edlinger wrote:
> >>>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
> >>>>
> >>>> PR middle-end/91603
> >>>> PR middle-end/91612
> >>>> PR middle-end/91613
> >>>> * expr.c (expand_expr_real_1): decl_p_1): Refactor into...
> >>>> (non_mem_decl_p): ...this.
> >>>> (mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
> >>>> (expand_assignment): Call mem_ref_referes_to_non_mem_p
> >>>> unconditionally as before.
> >>>
> >>> Not a review, just questioning the ChangeLog entry.
> >>> What is the "decl_p_1): " in there? Also, the ChangeLog mentions many
> >>> functions, but the patch in reality just modifies expand_expr_real_1
> >>> and nothing else.
> >>>
> >>
> >> Ah, sorry, this is of course wrong, (I forgot to complete the sentence,
> >> and later forgot to check it again)....
> >>
> >>
> >> This is what I actually wanted to say:
> >>
> >> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
> >>
> >> PR middle-end/91603
> >> PR middle-end/91612
> >> PR middle-end/91613
> >> * expr.c (expand_expr_real_1): Handle unaligned decl_rtl
> >> and SSA_NAME referring to CONSTANT_P correctly.
> >>
> >> testsuite:
> >> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
> >>
> >> PR middle-end/91603
> >> * testsuite/gcc.target/arm/pr91603.c: New test.
> >
> > @@ -10062,7 +10062,43 @@ expand_expr_real_1 (tree exp, rtx target,
> > machine_
> > {
> > if (exp && MEM_P (temp) && REG_P (XEXP (temp, 0)))
> > mark_reg_pointer (XEXP (temp, 0), DECL_ALIGN (exp));
> > + }
> > + else if (MEM_P (decl_rtl))
> > + temp = decl_rtl;
> >
> > + if (temp != 0)
> > + {
> > + if (MEM_P (temp)
> > + && modifier != EXPAND_WRITE
> > + && modifier != EXPAND_MEMORY
> > + && modifier != EXPAND_INITIALIZER
> > + && modifier != EXPAND_CONST_ADDRESS
> > + && modifier != EXPAND_SUM
> > + && !inner_reference_p
> > + && mode != BLKmode
> > + && MEM_ALIGN (temp) < GET_MODE_ALIGNMENT (mode))
> >
> > So other places ([TARGET_]MEM_REF cases) use "just"
> >
>
> Yes, interesting all of them do slightly different things.
> I started with cloning the MEM_REF case, but it ran immediately
> into issues with this assert here:
>
> result = expand_expr (exp, target, tmode,
> modifier == EXPAND_INITIALIZER
> ? EXPAND_INITIALIZER : EXPAND_CONST_ADDRESS);
>
> /* If the DECL isn't in memory, then the DECL wasn't properly
> marked TREE_ADDRESSABLE, which will be either a front-end
> or a tree optimizer bug. */
>
> gcc_assert (MEM_P (result));
> result = XEXP (result, 0);
>
> which implies that I need to add EXPAND_INITIALIZER and EXPAND_CONST_ADDRESS,
> but since the code immediately above also has an exception of EXPAND_SUM:
>
> else if (MEM_P (decl_rtl) && modifier != EXPAND_INITIALIZER)
> {
> if (alt_rtl)
> *alt_rtl = decl_rtl;
> decl_rtl = use_anchored_address (decl_rtl);
> if (modifier != EXPAND_CONST_ADDRESS
> && modifier != EXPAND_SUM
>
> I thought it I need to add also an exception for EXPAND_SUM.
>
> Probably there is a reason why TARGET_MEM_REF does not need the
> extract_bit_field stuff, when I read the comment here:
>
> /* If the target does not have special handling for unaligned
> loads of mode then it can use regular moves for them. */
> && ((icode = optab_handler (movmisalign_optab, mode))
> != CODE_FOR_nothing))
>
> it is just, I don't really believe it.
It should really be so. IVOPTs created them and asked the backend
if it supports it. But yeah - who knows, I'd have to double check
whether IVOPTs is careful here or not - at least I doubt targets
w/o movmisalign_optab will never create unaligned TARGET_MEM_REFs...
> > if (modifier != EXPAND_WRITE
> > && modifier != EXPAND_MEMORY
> > && !inner_reference_p
> > && mode != BLKmode
> > && align < GET_MODE_ALIGNMENT (mode))
> >
> > I also wonder if you can split out all this common code to
> > a function (the actual unaligned expansion, that is) and call
> > it from those places (where the TARGET_MEM_REF case misses the
> > slow_unaligned_access case - presumably wanting to "assert"
> > that this doesn't happen.
> >
> > /* If the target does not have special handling for unaligned
> > loads of mode then it can use regular moves for them. */
> >
>
> Actually there is still a small difference to the MEM_REF expansion,
> see the alt_rtl and the EXPAND_STACK_PARAM:
>
> temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
> 0, TYPE_UNSIGNED (TREE_TYPE (exp)),
> (modifier == EXPAND_STACK_PARM
> ? NULL_RTX : target),
> mode, mode, false, alt_rtl);
>
>
> TARGET_MEM_REF does not do extract_bit_field at all,
> while I think ignoring target and alt_rtl in the DECL_P case is safe,
> target, because it is at most a missed optimization, and
> alt_rtl because it should already be handled above?
> But if I pass target I cannot simply ignore alt_rtl any more?
Ick.
> Well, I could pass target and alt_rtl differently each time.
>
> should I still try to factor that into a single function, it will have
> around 7 parameters?
I'd have to see the result to say... but I did hope it was
going to be a bit simpler.
Richard.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)
2019-09-03 13:12 ` Richard Biener
@ 2019-09-03 13:13 ` Richard Biener
2019-09-03 13:31 ` Jeff Law
0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2019-09-03 13:13 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: Jakub Jelinek, gcc-patches, Jeff Law
On Tue, 3 Sep 2019, Richard Biener wrote:
> On Tue, 3 Sep 2019, Bernd Edlinger wrote:
>
> > On 9/3/19 1:12 PM, Richard Biener wrote:
> > > On Tue, 3 Sep 2019, Bernd Edlinger wrote:
> > >
> > >> On 9/3/19 9:05 AM, Jakub Jelinek wrote:
> > >>> On Tue, Sep 03, 2019 at 07:02:53AM +0000, Bernd Edlinger wrote:
> > >>>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
> > >>>>
> > >>>> PR middle-end/91603
> > >>>> PR middle-end/91612
> > >>>> PR middle-end/91613
> > >>>> * expr.c (expand_expr_real_1): decl_p_1): Refactor into...
> > >>>> (non_mem_decl_p): ...this.
> > >>>> (mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
> > >>>> (expand_assignment): Call mem_ref_referes_to_non_mem_p
> > >>>> unconditionally as before.
> > >>>
> > >>> Not a review, just questioning the ChangeLog entry.
> > >>> What is the "decl_p_1): " in there? Also, the ChangeLog mentions many
> > >>> functions, but the patch in reality just modifies expand_expr_real_1
> > >>> and nothing else.
> > >>>
> > >>
> > >> Ah, sorry, this is of course wrong, (I forgot to complete the sentence,
> > >> and later forgot to check it again)....
> > >>
> > >>
> > >> This is what I actually wanted to say:
> > >>
> > >> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
> > >>
> > >> PR middle-end/91603
> > >> PR middle-end/91612
> > >> PR middle-end/91613
> > >> * expr.c (expand_expr_real_1): Handle unaligned decl_rtl
> > >> and SSA_NAME referring to CONSTANT_P correctly.
> > >>
> > >> testsuite:
> > >> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
> > >>
> > >> PR middle-end/91603
> > >> * testsuite/gcc.target/arm/pr91603.c: New test.
> > >
> > > @@ -10062,7 +10062,43 @@ expand_expr_real_1 (tree exp, rtx target,
> > > machine_
> > > {
> > > if (exp && MEM_P (temp) && REG_P (XEXP (temp, 0)))
> > > mark_reg_pointer (XEXP (temp, 0), DECL_ALIGN (exp));
> > > + }
> > > + else if (MEM_P (decl_rtl))
> > > + temp = decl_rtl;
> > >
> > > + if (temp != 0)
> > > + {
> > > + if (MEM_P (temp)
> > > + && modifier != EXPAND_WRITE
> > > + && modifier != EXPAND_MEMORY
> > > + && modifier != EXPAND_INITIALIZER
> > > + && modifier != EXPAND_CONST_ADDRESS
> > > + && modifier != EXPAND_SUM
> > > + && !inner_reference_p
> > > + && mode != BLKmode
> > > + && MEM_ALIGN (temp) < GET_MODE_ALIGNMENT (mode))
> > >
> > > So other places ([TARGET_]MEM_REF cases) use "just"
> > >
> >
> > Yes, interesting all of them do slightly different things.
> > I started with cloning the MEM_REF case, but it ran immediately
> > into issues with this assert here:
> >
> > result = expand_expr (exp, target, tmode,
> > modifier == EXPAND_INITIALIZER
> > ? EXPAND_INITIALIZER : EXPAND_CONST_ADDRESS);
> >
> > /* If the DECL isn't in memory, then the DECL wasn't properly
> > marked TREE_ADDRESSABLE, which will be either a front-end
> > or a tree optimizer bug. */
> >
> > gcc_assert (MEM_P (result));
> > result = XEXP (result, 0);
> >
> > which implies that I need to add EXPAND_INITIALIZER and EXPAND_CONST_ADDRESS,
> > but since the code immediately above also has an exception of EXPAND_SUM:
> >
> > else if (MEM_P (decl_rtl) && modifier != EXPAND_INITIALIZER)
> > {
> > if (alt_rtl)
> > *alt_rtl = decl_rtl;
> > decl_rtl = use_anchored_address (decl_rtl);
> > if (modifier != EXPAND_CONST_ADDRESS
> > && modifier != EXPAND_SUM
> >
> > I thought it I need to add also an exception for EXPAND_SUM.
> >
> > Probably there is a reason why TARGET_MEM_REF does not need the
> > extract_bit_field stuff, when I read the comment here:
> >
> > /* If the target does not have special handling for unaligned
> > loads of mode then it can use regular moves for them. */
> > && ((icode = optab_handler (movmisalign_optab, mode))
> > != CODE_FOR_nothing))
> >
> > it is just, I don't really believe it.
>
> It should really be so. IVOPTs created them and asked the backend
> if it supports it. But yeah - who knows, I'd have to double check
> whether IVOPTs is careful here or not - at least I doubt targets
> w/o movmisalign_optab will never create unaligned TARGET_MEM_REFs...
>
> > > if (modifier != EXPAND_WRITE
> > > && modifier != EXPAND_MEMORY
> > > && !inner_reference_p
> > > && mode != BLKmode
> > > && align < GET_MODE_ALIGNMENT (mode))
> > >
> > > I also wonder if you can split out all this common code to
> > > a function (the actual unaligned expansion, that is) and call
> > > it from those places (where the TARGET_MEM_REF case misses the
> > > slow_unaligned_access case - presumably wanting to "assert"
> > > that this doesn't happen.
> > >
> > > /* If the target does not have special handling for unaligned
> > > loads of mode then it can use regular moves for them. */
> > >
> >
> > Actually there is still a small difference to the MEM_REF expansion,
> > see the alt_rtl and the EXPAND_STACK_PARAM:
> >
> > temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
> > 0, TYPE_UNSIGNED (TREE_TYPE (exp)),
> > (modifier == EXPAND_STACK_PARM
> > ? NULL_RTX : target),
> > mode, mode, false, alt_rtl);
> >
> >
> > TARGET_MEM_REF does not do extract_bit_field at all,
> > while I think ignoring target and alt_rtl in the DECL_P case is safe,
> > target, because it is at most a missed optimization, and
> > alt_rtl because it should already be handled above?
> > But if I pass target I cannot simply ignore alt_rtl any more?
>
> Ick.
>
> > Well, I could pass target and alt_rtl differently each time.
> >
> > should I still try to factor that into a single function, it will have
> > around 7 parameters?
>
> I'd have to see the result to say... but I did hope it was
> going to be a bit simpler.
I'd say go for the original patch and try the refactoring on top.
Thus, OK.
Thanks,
Richard.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)
2019-09-03 13:13 ` Richard Biener
@ 2019-09-03 13:31 ` Jeff Law
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Law @ 2019-09-03 13:31 UTC (permalink / raw)
To: Richard Biener, Bernd Edlinger; +Cc: Jakub Jelinek, gcc-patches
On 9/3/19 7:13 AM, Richard Biener wrote:
> On Tue, 3 Sep 2019, Richard Biener wrote:
>
>> On Tue, 3 Sep 2019, Bernd Edlinger wrote:
>>
>>> On 9/3/19 1:12 PM, Richard Biener wrote:
>>>> On Tue, 3 Sep 2019, Bernd Edlinger wrote:
>>>>
>>>>> On 9/3/19 9:05 AM, Jakub Jelinek wrote:
>>>>>> On Tue, Sep 03, 2019 at 07:02:53AM +0000, Bernd Edlinger wrote:
>>>>>>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>>>>>
>>>>>>> PR middle-end/91603
>>>>>>> PR middle-end/91612
>>>>>>> PR middle-end/91613
>>>>>>> * expr.c (expand_expr_real_1): decl_p_1): Refactor into...
>>>>>>> (non_mem_decl_p): ...this.
>>>>>>> (mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
>>>>>>> (expand_assignment): Call mem_ref_referes_to_non_mem_p
>>>>>>> unconditionally as before.
>>>>>>
>>>>>> Not a review, just questioning the ChangeLog entry.
>>>>>> What is the "decl_p_1): " in there? Also, the ChangeLog mentions many
>>>>>> functions, but the patch in reality just modifies expand_expr_real_1
>>>>>> and nothing else.
>>>>>>
>>>>>
>>>>> Ah, sorry, this is of course wrong, (I forgot to complete the sentence,
>>>>> and later forgot to check it again)....
>>>>>
>>>>>
>>>>> This is what I actually wanted to say:
>>>>>
>>>>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>>>
>>>>> PR middle-end/91603
>>>>> PR middle-end/91612
>>>>> PR middle-end/91613
>>>>> * expr.c (expand_expr_real_1): Handle unaligned decl_rtl
>>>>> and SSA_NAME referring to CONSTANT_P correctly.
>>>>>
>>>>> testsuite:
>>>>> 2019-09-03 Bernd Edlinger <bernd.edlinger@hotmail.de>
>>>>>
>>>>> PR middle-end/91603
>>>>> * testsuite/gcc.target/arm/pr91603.c: New test.
>>>>
>>>> @@ -10062,7 +10062,43 @@ expand_expr_real_1 (tree exp, rtx target,
>>>> machine_
>>>> {
>>>> if (exp && MEM_P (temp) && REG_P (XEXP (temp, 0)))
>>>> mark_reg_pointer (XEXP (temp, 0), DECL_ALIGN (exp));
>>>> + }
>>>> + else if (MEM_P (decl_rtl))
>>>> + temp = decl_rtl;
>>>>
>>>> + if (temp != 0)
>>>> + {
>>>> + if (MEM_P (temp)
>>>> + && modifier != EXPAND_WRITE
>>>> + && modifier != EXPAND_MEMORY
>>>> + && modifier != EXPAND_INITIALIZER
>>>> + && modifier != EXPAND_CONST_ADDRESS
>>>> + && modifier != EXPAND_SUM
>>>> + && !inner_reference_p
>>>> + && mode != BLKmode
>>>> + && MEM_ALIGN (temp) < GET_MODE_ALIGNMENT (mode))
>>>>
>>>> So other places ([TARGET_]MEM_REF cases) use "just"
>>>>
>>>
>>> Yes, interesting all of them do slightly different things.
>>> I started with cloning the MEM_REF case, but it ran immediately
>>> into issues with this assert here:
>>>
>>> result = expand_expr (exp, target, tmode,
>>> modifier == EXPAND_INITIALIZER
>>> ? EXPAND_INITIALIZER : EXPAND_CONST_ADDRESS);
>>>
>>> /* If the DECL isn't in memory, then the DECL wasn't properly
>>> marked TREE_ADDRESSABLE, which will be either a front-end
>>> or a tree optimizer bug. */
>>>
>>> gcc_assert (MEM_P (result));
>>> result = XEXP (result, 0);
>>>
>>> which implies that I need to add EXPAND_INITIALIZER and EXPAND_CONST_ADDRESS,
>>> but since the code immediately above also has an exception of EXPAND_SUM:
>>>
>>> else if (MEM_P (decl_rtl) && modifier != EXPAND_INITIALIZER)
>>> {
>>> if (alt_rtl)
>>> *alt_rtl = decl_rtl;
>>> decl_rtl = use_anchored_address (decl_rtl);
>>> if (modifier != EXPAND_CONST_ADDRESS
>>> && modifier != EXPAND_SUM
>>>
>>> I thought it I need to add also an exception for EXPAND_SUM.
>>>
>>> Probably there is a reason why TARGET_MEM_REF does not need the
>>> extract_bit_field stuff, when I read the comment here:
>>>
>>> /* If the target does not have special handling for unaligned
>>> loads of mode then it can use regular moves for them. */
>>> && ((icode = optab_handler (movmisalign_optab, mode))
>>> != CODE_FOR_nothing))
>>>
>>> it is just, I don't really believe it.
>>
>> It should really be so. IVOPTs created them and asked the backend
>> if it supports it. But yeah - who knows, I'd have to double check
>> whether IVOPTs is careful here or not - at least I doubt targets
>> w/o movmisalign_optab will never create unaligned TARGET_MEM_REFs...
>>
>>>> if (modifier != EXPAND_WRITE
>>>> && modifier != EXPAND_MEMORY
>>>> && !inner_reference_p
>>>> && mode != BLKmode
>>>> && align < GET_MODE_ALIGNMENT (mode))
>>>>
>>>> I also wonder if you can split out all this common code to
>>>> a function (the actual unaligned expansion, that is) and call
>>>> it from those places (where the TARGET_MEM_REF case misses the
>>>> slow_unaligned_access case - presumably wanting to "assert"
>>>> that this doesn't happen.
>>>>
>>>> /* If the target does not have special handling for unaligned
>>>> loads of mode then it can use regular moves for them. */
>>>>
>>>
>>> Actually there is still a small difference to the MEM_REF expansion,
>>> see the alt_rtl and the EXPAND_STACK_PARAM:
>>>
>>> temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
>>> 0, TYPE_UNSIGNED (TREE_TYPE (exp)),
>>> (modifier == EXPAND_STACK_PARM
>>> ? NULL_RTX : target),
>>> mode, mode, false, alt_rtl);
>>>
>>>
>>> TARGET_MEM_REF does not do extract_bit_field at all,
>>> while I think ignoring target and alt_rtl in the DECL_P case is safe,
>>> target, because it is at most a missed optimization, and
>>> alt_rtl because it should already be handled above?
>>> But if I pass target I cannot simply ignore alt_rtl any more?
>>
>> Ick.
>>
>>> Well, I could pass target and alt_rtl differently each time.
>>>
>>> should I still try to factor that into a single function, it will have
>>> around 7 parameters?
>>
>> I'd have to see the result to say... but I did hope it was
>> going to be a bit simpler.
>
> I'd say go for the original patch and try the refactoring on top.
Works for me as well.
jeff
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-03 13:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-03 7:02 [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613) Bernd Edlinger
2019-09-03 7:05 ` Jakub Jelinek
2019-09-03 7:11 ` Bernd Edlinger
2019-09-03 11:12 ` Richard Biener
2019-09-03 13:02 ` Bernd Edlinger
2019-09-03 13:12 ` Richard Biener
2019-09-03 13:13 ` Richard Biener
2019-09-03 13:31 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).