Re: [PATCH] middle-end/95493 - bogus MEM_ATTRS for variable array access
On Thu, Jun 04, 2020 at 01:52:53PM +0200, Richard Biener wrote:
> * g++.dg/torture/pr95493.C: New testcase.
> +using K [[gnu::vector_size(16)]] = int;
...
> +int a = [](K y) {
> + for (int j = 0; j < 4; ++j)
> +if (y[j] != 0)
> + return j;
> + return -1;
> +}(x.d);
The lambda has SSE vector argument, so on i686-linux or otherwise with
-mno-sse we get a warning at -O0 when the lambda isn't inlined.
Fixed thusly, tested on x86_64-linux, committed to trunk as obvious:
2020-06-09 Jakub Jelinek
* g++.dg/torture/pr95493.C: Add -Wno-psabi -w to dg-additional-options.
--- gcc/testsuite/g++.dg/torture/pr95493.C.jj 2020-06-05 10:42:40.635895301
+0200
+++ gcc/testsuite/g++.dg/torture/pr95493.C 2020-06-09 08:32:47.220113011
+0200
@@ -1,5 +1,5 @@
// { dg-do run }
-// { dg-additional-options "-std=c++17" }
+// { dg-additional-options "-std=c++17 -Wno-psabi -w" }
struct verify
{
Jakub
Re: [PATCH] middle-end/95493 - bogus MEM_ATTRS for variable array access
On June 5, 2020 6:38:10 PM GMT+02:00, Eric Botcazou wrote: >> I've installed it on trunk but will give it quite a while there >before >> backporting. I'm still somewhat worried about the >> >> /* ??? If we end up with a constant or a descriptor do not >> record a MEM_EXPR. */ >> else if (CONSTANT_CLASS_P (t) >> >>|| TREE_CODE (t) == CONSTRUCTOR) >> >> ; >> >> case, maybe we should assert that bitpos == 0 here since we're >> dropping it on the floor but eventually inherit offset_known_p == >true >> from the already present MEM_ATTRs. > >It's hard to imagine without a testcase how you can end up inheriting >anything >for CONSTANT_CLASS_P or CONSTRUCTOR though: where do the MEM_ATTRs come >from? For constructor it was constant descriptors seen during ada bootstrap. I can imagine constants are similar for constant pool entries. Richard.
Re: [PATCH] middle-end/95493 - bogus MEM_ATTRS for variable array access
> I've installed it on trunk but will give it quite a while there before > backporting. I'm still somewhat worried about the > > /* ??? If we end up with a constant or a descriptor do not > record a MEM_EXPR. */ > else if (CONSTANT_CLASS_P (t) > >|| TREE_CODE (t) == CONSTRUCTOR) > > ; > > case, maybe we should assert that bitpos == 0 here since we're > dropping it on the floor but eventually inherit offset_known_p == true > from the already present MEM_ATTRs. It's hard to imagine without a testcase how you can end up inheriting anything for CONSTANT_CLASS_P or CONSTRUCTOR though: where do the MEM_ATTRs come from? -- Eric Botcazou
Re: [PATCH] middle-end/95493 - bogus MEM_ATTRS for variable array access
On Fri, 5 Jun 2020, Eric Botcazou wrote: > > The patch ends up recording the whole Array ref with variable index. All > > alias analysis code deals with this just fine. IIRC historically we tried > > to save memory with stripping and dropping of MEM_EXPRs. > > OK, I agree that the cleanup makes sense these days and can probably also be > backported onto the 10 branch without too much risk. I've installed it on trunk but will give it quite a while there before backporting. I'm still somewhat worried about the /* ??? If we end up with a constant or a descriptor do not record a MEM_EXPR. */ else if (CONSTANT_CLASS_P (t) || TREE_CODE (t) == CONSTRUCTOR) ; case, maybe we should assert that bitpos == 0 here since we're dropping it on the floor but eventually inherit offset_known_p == true from the already present MEM_ATTRs. I guess set_mem_attributes_minus_bitpos should be an all-or-nothing with regard to the inherited MEM_ATTRs - take them unmodified or override them completely. But even then - can we simply ignore the passed in bitpos? shall we at least set offset_known_p to false if we do? We're also happily ignoring an inherited attrs.offset. Still this all suggests the take it all or nothing approach. Also for the one case I've seen (through expand_debug_expr though - "semantically" irreleveant thus) where we end up being called for a MEM that was generated for a completely different object, for example a stack slot, but we'll still apply things like alignment analysis on the passed in tree which might be unrelated enough to make the derived alignment invalid for the actual MEM. Richard.
Re: [PATCH] middle-end/95493 - bogus MEM_ATTRS for variable array access
> The patch ends up recording the whole Array ref with variable index. All > alias analysis code deals with this just fine. IIRC historically we tried > to save memory with stripping and dropping of MEM_EXPRs. OK, I agree that the cleanup makes sense these days and can probably also be backported onto the 10 branch without too much risk. -- Eric Botcazou
Re: [PATCH] middle-end/95493 - bogus MEM_ATTRS for variable array access
On June 4, 2020 6:20:36 PM GMT+02:00, Eric Botcazou wrote: >> I'll go with this variant since it is more obvious unless I hear >> otherwise. > >Yes, at least this one doesn't appear to further confuse an already >confusing >implementation. ;-) > >> Thanks, >> Richard. >> >> >> The following patch avoids keeping the inherited MEM_ATTRS when >> set_mem_attributes_minus_bitpos is called with a variable ARRAY_REF. >> The inherited ones may not reflect the correct offset and neither >> does the updated alias-set match the inherited MEM_EXPR. This all >> ends up confusing path-based alias-analysis, causing wrong-code. > >"variable ARRAY_REF" is just an ARRAY_REF with variable index or an >ARRAY_REF >whose base is variable? In other words, do we end up taking the > /* Else do not record a MEM_EXPR. */ > >path instead of setting attrs.expr again? Yes, we're taking this path in the problematical case. >> The fix is to stop not adopting a MEM_EXPR for certain kinds of >> expressions and instead handle everything we can. There's still >> the constant kind trees case which I'm too lazy to look into right >> now. I did refrain from adding SSA_NAME there and instead avoided >> calling set_mem_attributes_minus_bitpos when debug expression >> expansion ended up expanding a SSA definition RHS which should >> already have taken care of setting the appropriate MEM_ATTRS. > >Do we really need to ditch the entire ARRAY_REF path though? Aren't we >going >to lose some (trivial) disambiguation possibilities later on by >recording the >ARRAY_REF instead of DECL or COMPONENT_REF (+ offset)? The patch ends up recording the whole Array ref with variable index. All alias analysis code deals with this just fine. IIRC historically we tried to save memory with stripping and dropping of MEM_EXPRs. Richard.
Re: [PATCH] middle-end/95493 - bogus MEM_ATTRS for variable array access
> I'll go with this variant since it is more obvious unless I hear > otherwise. Yes, at least this one doesn't appear to further confuse an already confusing implementation. ;-) > Thanks, > Richard. > > > The following patch avoids keeping the inherited MEM_ATTRS when > set_mem_attributes_minus_bitpos is called with a variable ARRAY_REF. > The inherited ones may not reflect the correct offset and neither > does the updated alias-set match the inherited MEM_EXPR. This all > ends up confusing path-based alias-analysis, causing wrong-code. "variable ARRAY_REF" is just an ARRAY_REF with variable index or an ARRAY_REF whose base is variable? In other words, do we end up taking the /* Else do not record a MEM_EXPR. */ path instead of setting attrs.expr again? > The fix is to stop not adopting a MEM_EXPR for certain kinds of > expressions and instead handle everything we can. There's still > the constant kind trees case which I'm too lazy to look into right > now. I did refrain from adding SSA_NAME there and instead avoided > calling set_mem_attributes_minus_bitpos when debug expression > expansion ended up expanding a SSA definition RHS which should > already have taken care of setting the appropriate MEM_ATTRS. Do we really need to ditch the entire ARRAY_REF path though? Aren't we going to lose some (trivial) disambiguation possibilities later on by recording the ARRAY_REF instead of DECL or COMPONENT_REF (+ offset)? -- Eric Botcazou
[PATCH] middle-end/95493 - bogus MEM_ATTRS for variable array access
So this is a variant cleaning up set_mem_attributes_minus_bitpos
instead.
Bootstrapped and tested on x86_64-unknown-linux-gnu.
I'll go with this variant since it is more obvious unless I hear
otherwise.
Thanks,
Richard.
The following patch avoids keeping the inherited MEM_ATTRS when
set_mem_attributes_minus_bitpos is called with a variable ARRAY_REF.
The inherited ones may not reflect the correct offset and neither
does the updated alias-set match the inherited MEM_EXPR. This all
ends up confusing path-based alias-analysis, causing wrong-code.
The fix is to stop not adopting a MEM_EXPR for certain kinds of
expressions and instead handle everything we can. There's still
the constant kind trees case which I'm too lazy to look into right
now. I did refrain from adding SSA_NAME there and instead avoided
calling set_mem_attributes_minus_bitpos when debug expression
expansion ended up expanding a SSA definition RHS which should
already have taken care of setting the appropriate MEM_ATTRS.
2020-06-04 Richard Biener
PR middle-end/95493
* cfgexpand.c (expand_debug_expr): Avoid calling
set_mem_attributes_minus_bitpos when we were expanding
an SSA name.
* emit-rtl.c (set_mem_attributes_minus_bitpos): Remove
ARRAY_REF special-casing, add CONSTRUCTOR to the set of
special-cases we do not want MEM_EXPRs for. Assert
we end up with reasonable MEM_EXPRs.
* g++.dg/torture/pr95493.C: New testcase.
---
gcc/cfgexpand.c| 3 +-
gcc/emit-rtl.c | 63 --
gcc/testsuite/g++.dg/torture/pr95493.C | 62 +
3 files changed, 73 insertions(+), 55 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/torture/pr95493.C
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 2f6ec97ed04..b270a4ddb9d 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -4616,7 +4616,8 @@ expand_debug_expr (tree exp)
op0 = copy_rtx (op0);
if (op0 == orig_op0)
op0 = shallow_copy_rtx (op0);
- set_mem_attributes (op0, exp, 0);
+ if (TREE_CODE (tem) != SSA_NAME)
+ set_mem_attributes (op0, exp, 0);
}
if (known_eq (bitpos, 0) && mode == GET_MODE (op0))
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 2b790636366..f9b0e9714d9 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -2067,8 +2067,10 @@ set_mem_attributes_minus_bitpos (rtx ref, tree t, int
objectp,
new_size = DECL_SIZE_UNIT (t);
}
- /* ??? If we end up with a constant here do record a MEM_EXPR. */
- else if (CONSTANT_CLASS_P (t))
+ /* ??? If we end up with a constant or a descriptor do not
+record a MEM_EXPR. */
+ else if (CONSTANT_CLASS_P (t)
+ || TREE_CODE (t) == CONSTRUCTOR)
;
/* If this is a field reference, record it. */
@@ -2082,59 +2084,12 @@ set_mem_attributes_minus_bitpos (rtx ref, tree t, int
objectp,
new_size = DECL_SIZE_UNIT (TREE_OPERAND (t, 1));
}
- /* If this is an array reference, look for an outer field reference. */
- else if (TREE_CODE (t) == ARRAY_REF)
- {
- tree off_tree = size_zero_node;
- /* We can't modify t, because we use it at the end of the
-function. */
- tree t2 = t;
-
- do
- {
- tree index = TREE_OPERAND (t2, 1);
- tree low_bound = array_ref_low_bound (t2);
- tree unit_size = array_ref_element_size (t2);
-
- /* We assume all arrays have sizes that are a multiple of a byte.
-First subtract the lower bound, if any, in the type of the
-index, then convert to sizetype and multiply by the size of
-the array element. */
- if (! integer_zerop (low_bound))
- index = fold_build2 (MINUS_EXPR, TREE_TYPE (index),
-index, low_bound);
-
- off_tree = size_binop (PLUS_EXPR,
-size_binop (MULT_EXPR,
-fold_convert (sizetype,
- index),
-unit_size),
-off_tree);
- t2 = TREE_OPERAND (t2, 0);
- }
- while (TREE_CODE (t2) == ARRAY_REF);
-
- if (DECL_P (t2)
- || (TREE_CODE (t2) == COMPONENT_REF
- /* For trailing arrays t2 doesn't have a size that
-covers all valid accesses. */
- && ! array_at_struct_end_p (t)))
- {
- attrs.expr = t2;
- attrs.offset_known_p = false;
- if (poly_int_tree_p (off_tree, ))
- {
- attrs.offset_known_p = true;
- apply_bitpos = bitpos;
-
