diff mbox series

SRA: Force gimple operand in an additional corner case (PR 112822)

Message ID ri6wmtjcqlw.fsf@
State New
Headers show
Series SRA: Force gimple operand in an additional corner case (PR 112822) | expand

Commit Message

Martin Jambor Dec. 12, 2023, 4:50 p.m. UTC 
Hi,

PR 112822 revealed a corner case in load_assign_lhs_subreplacements
where it creates invalid gimple: an assignment where on the LHS there
is a complex variable which however is not a gimple register because
it has partial defs and on the right hand side there is a
VIEW_CONVERT_EXPR.  This patch invokes force_gimple_operand_gsi on
such statements (like it already does when both sides of a generated
assignment have partial definitions.

I've made sure the patch passes bootstrap and testsuite on x86_64-linux,
the bug reporter was kind enough to also check the same on an
powerpc64le-linux (see bugzilla comment #8).

The testcase has reasonable size but it is specific to ppc64le and its
altivec vectors.  My plan is to ask the bug reporter to massage it into
a target specific testcase in bugzilla.  Alternatively I can try to
craft a testcase from scratch but that will take time.

Despite the above, is the patch OK for master?

Thanks,

Martin



gcc/ChangeLog:

2023-12-12  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/112822
	* tree-sra.cc (load_assign_lhs_subreplacements): Invoke
	force_gimple_operand_gsi also when LHS has partial stores and RHS is a
	VIEW_CONVERT_EXPR.
---
 gcc/tree-sra.cc | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Richard Biener Dec. 12, 2023, 5:17 p.m. UTC | #1 
> Am 12.12.2023 um 17:50 schrieb Martin Jambor <mjambor@suse.cz>:
> 
> Hi,
> 
> PR 112822 revealed a corner case in load_assign_lhs_subreplacements
> where it creates invalid gimple: an assignment where on the LHS there
> is a complex variable which however is not a gimple register because
> it has partial defs and on the right hand side there is a
> VIEW_CONVERT_EXPR.  This patch invokes force_gimple_operand_gsi on
> such statements (like it already does when both sides of a generated
> assignment have partial definitions.
> 
> I've made sure the patch passes bootstrap and testsuite on x86_64-linux,
> the bug reporter was kind enough to also check the same on an
> powerpc64le-linux (see bugzilla comment #8).
> 
> The testcase has reasonable size but it is specific to ppc64le and its
> altivec vectors.  My plan is to ask the bug reporter to massage it into
> a target specific testcase in bugzilla.  Alternatively I can try to
> craft a testcase from scratch but that will take time.
> 
> Despite the above, is the patch OK for master?

Ok

Richard 

> 
> Thanks,
> 
> Martin
> 
> 
> 
> gcc/ChangeLog:
> 
> 2023-12-12  Martin Jambor  <mjambor@suse.cz>
> 
>    PR tree-optimization/112822
>    * tree-sra.cc (load_assign_lhs_subreplacements): Invoke
>    force_gimple_operand_gsi also when LHS has partial stores and RHS is a
>    VIEW_CONVERT_EXPR.
> ---
> gcc/tree-sra.cc | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> index 3bd0c7a9af0..99a1b0a6d17 100644
> --- a/gcc/tree-sra.cc
> +++ b/gcc/tree-sra.cc
> @@ -4219,11 +4219,15 @@ load_assign_lhs_subreplacements (struct access *lacc,
>      if (racc && racc->grp_to_be_replaced)
>        {
>          rhs = get_access_replacement (racc);
> +          bool vce = false;
>          if (!useless_type_conversion_p (lacc->type, racc->type))
> -        rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR,
> -                       lacc->type, rhs);
> +        {
> +          rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR,
> +                     lacc->type, rhs);
> +          vce = true;
> +        }
> 
> -          if (racc->grp_partial_lhs && lacc->grp_partial_lhs)
> +          if (lacc->grp_partial_lhs && (vce || racc->grp_partial_lhs))
>        rhs = force_gimple_operand_gsi (&sad->old_gsi, rhs, true,
>                        NULL_TREE, true, GSI_SAME_STMT);
>        }
> --
> 2.43.0
>
Peter Bergner Dec. 12, 2023, 6:45 p.m. UTC | #2 
On 12/12/23 10:50 AM, Martin Jambor wrote:
> The testcase has reasonable size but it is specific to ppc64le and its
> altivec vectors.  My plan is to ask the bug reporter to massage it into
> a target specific testcase in bugzilla.  Alternatively I can try to
> craft a testcase from scratch but that will take time.

I rewrote the Altivec specific part of the testcase to use generic C code
and it still ICEs for me on ppc64le using an unpatched compiler.  Therefore,
I think we can just add the updated testcase to the generic g++ tests. 

I'll note I was wrong in the bugzilla comments, -O3 -mcpu=power10 is not
required to hit the ICE.  A simple -O2 on ppc64le is enough to hit the ICE.

Ok for trunk?

Peter


testsuite: Add testcase for already fixed PR [PR112822]

gcc/testsuite/
	PR tree-optimization/112822
	* g++.dg/pr112822.C: New test.

diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C
new file mode 100644
index 00000000000..3921d5c1bbe
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr112822.C
@@ -0,0 +1,369 @@
+/* PR target/112822 */
+/* { dg-options "-w -O2" } */
+
+/* Verify we do not ICE on the following noisy creduced test case.  */
+
+namespace b {
+typedef int c;
+template <bool, typename> struct d;
+template <typename e> struct d<true, e> { using f = e; };
+template <bool, typename, typename> struct aa;
+template <typename g, typename h> struct aa<false, g, h> { using f = h; };
+template <bool l, typename e> using ab = typename d<l, e>::f;
+template <bool l, typename g, typename h> using n = typename aa<l, g, h>::f;
+template <typename> class af {
+public:
+  typedef __complex__ ah;
+  template <typename e> af operator+=(e) {
+    ah o;
+    x = o;
+    return *this;
+  }
+  ah x;
+};
+} // namespace b
+namespace {
+enum { p };
+enum { ac, ad };
+struct ae;
+struct al;
+struct ag;
+typedef b::c an;
+namespace ai {
+template <typename aj> struct ak { typedef aj f; };
+template <typename aj> using ar = typename ak<aj>::f;
+template <typename> struct am {
+  enum { at };
+};
+template <typename, typename> struct ao {
+  enum { at };
+};
+template <typename> struct ap;
+template <typename> struct aq {
+  enum { at };
+};
+} // namespace ai
+template <typename> struct ay;
+template <typename> class as;
+template <typename, int> class ba;
+template <typename, int au, int av, int = 0, int = au, int = av> class aw;
+template <typename> class be;
+template <typename, typename, int> class az;
+namespace ai {
+template <typename, typename> struct bg;
+template <typename aj, int = bg<typename aj::bb, typename aj::bc>::bd>
+struct bk;
+template <typename, typename> struct bf;
+template <typename> struct bm;
+template <typename> struct bh;
+template <int, typename bi, bool = ao<bi, typename bh<bi>::bj>::at> struct bp {
+  typedef bi f;
+};
+template <typename aj, int bl> struct br {
+  typedef typename bp<bl, typename bm<aj>::f>::f f;
+};
+template <typename, typename, int> struct bn;
+template <typename aj, int bo> struct bn<aj, al, bo> {
+  typedef aw<typename aj ::bu, aj ::bv, aj ::bq> f;
+};
+template <typename aj> struct bx {
+  typedef typename bn<aj, typename ap<aj>::bs, aj ::bo>::f f;
+};
+template <typename aj> struct bt { typedef b::n<0, aj, aj> f; };
+template <typename aj, int, typename ca = typename bx<aj>::f> struct cb {
+  enum { bw };
+  typedef b::n<bw, ca, typename bt<aj>::f> f;
+};
+template <typename cd, typename = typename ap<cd>::bs> struct by {
+  typedef be<cd> f;
+};
+template <typename cd, typename bs> struct bz {
+  typedef typename by<cd, bs>::f f;
+};
+template <typename, typename, int> struct ch;
+template <typename ci, int cc> struct ch<ci, ci, cc> { typedef ci bd; };
+} // namespace ai
+template <typename ck, typename ce, typename = ai::bf<ck, ce>> struct cg;
+template <typename aj, typename cm> struct cg<aj, cm> { typedef aj cn; };
+namespace ai {
+template <typename cj, int> cj cp;
+template <typename bu, typename cj, int> void cl(bu *cr, cj cs) { ct(cr, cs); }
+typedef __attribute__((altivec(vector__))) double co;
+void ct(double *cr, co cs) { *(co *)cr = cs; }
+struct cq {
+  co q;
+};
+template <> struct bm<b::af<double>> { typedef cq f; };
+template <> struct bh<cq> { typedef cq bj; };
+void ct(b::af<double> *cr, cq cs) { ct((double *)cr, cs.q); }
+template <typename cw, typename> struct cx {
+  template <int cy, typename cj> void cu(cw *a, cj) {
+    cl<cw, cj, cy>(a, cp<cj, cy>);
+  }
+};
+} // namespace ai
+template <typename cd> class ba<cd, ac> : public ay<cd> {
+public:
+  typedef ai::ap<cd> bu;
+  typedef b::n<ai::ap<cd>::bo, bu, b::n<ai::am<bu>::at, bu, bu>> cv;
+  typedef ay<cd> db;
+  db::dc;
+  cv coeff(an dd, an col) const { return dc().coeff(dd, col); }
+};
+template <typename cd> class cz : public ba<cd, ai::aq<cd>::at> {
+public:
+  ai::ap<cd> b;
+  enum { da, dg, dh, bv, bq, di = dg, bo };
+};
+template <typename cd> class be : public cz<cd> {
+public:
+  typedef typename ai::ap<cd>::bu bu;
+  typedef cz<cd> db;
+  db::dc;
+  template <typename de> cd &operator+=(const be<de> &);
+  template <typename de> az<cd, de, ad> df(de);
+};
+template <typename cd> struct ay {
+  cd &dc() { return *static_cast<cd *>(this); }
+  cd dc() const;
+};
+template <typename, typename, int, typename> class dl;
+namespace ai {
+template <typename bb, typename bc, int dm> struct ap<az<bb, bc, dm>> {
+  typedef bb dj;
+  typedef bc r;
+  typedef ap<dj> s;
+  typedef ap<r> t;
+  typedef typename cg<typename dj ::bu, typename r ::bu>::cn bu;
+  typedef typename ch<typename s::cf, typename t::cf, bg<bb, bc>::bd>::bd cf;
+  enum { bo };
+};
+} // namespace ai
+template <typename dk, typename Rhs_, int dm>
+class az : public dl<dk, Rhs_, dm,
+                     ai::ch<ai::ap<dk>, ai::ap<Rhs_>, ai::bg<dk, Rhs_>::bd>> {
+public:
+  typedef dk bb;
+  typedef Rhs_ bc;
+  typedef typename ai::bt<bb>::f LhsNested;
+  typedef typename ai::bt<bc>::f dn;
+  typedef ai::ar<LhsNested> u;
+  typedef ai::ar<dn> RhsNestedCleaned;
+  u lhs();
+  RhsNestedCleaned rhs();
+};
+template <typename bb, typename bc, int dm, typename>
+class dl : public ai::bz<az<bb, bc, dm>, al>::f {};
+namespace ai {
+template <typename> struct v { typedef ag w; };
+template <typename aj> struct evaluator_traits_base {
+  typedef typename v<typename ap<aj>::cf>::w w;
+};
+template <typename aj> struct ax : evaluator_traits_base<aj> {};
+template <typename> struct y { static const bool at = false; };
+template <typename bu, int z> class plainobjectbase_evaluator_data {
+public:
+  plainobjectbase_evaluator_data(bu *ptr, an) : data(ptr) {}
+  an outerStride() { return z; }
+  bu *data;
+};
+template <typename cd> struct evaluator {
+  typedef cd PlainObjectType;
+  typedef typename PlainObjectType::bu bu;
+  enum { IsVectorAtCompileTime };
+  enum { OuterStrideAtCompileTime };
+  evaluator(PlainObjectType &m) : m_d(m.data(), IsVectorAtCompileTime) {}
+  bu &coeffRef(an, an) { return m_d.data[m_d.outerStride()]; }
+  plainobjectbase_evaluator_data<bu, OuterStrideAtCompileTime> m_d;
+};
+template <typename bu, int Rows, int Cols, int Options, int MaxRows,
+          int MaxCols>
+struct evaluator<aw<bu, Rows, Cols, Options, MaxRows, MaxCols>>
+    : evaluator<as<aw<bu, Rows, Cols>>> {
+  typedef aw<bu, Rows, Cols> XprType;
+  evaluator(XprType &m) : evaluator<as<XprType>>(m) {}
+};
+template <typename DstEvaluator, typename, typename>
+struct copy_using_evaluator_traits {
+  typedef typename DstEvaluator::bu cw;
+  enum { RestrictedInnerSize };
+  typedef typename br<cw, RestrictedInnerSize>::f bi;
+};
+template <typename Kernel, an, int>
+struct copy_using_evaluator_innervec_CompleteUnrolling {
+  typedef typename Kernel::bi bi;
+  enum { outer, inner, SrcAlignment, DstAlignment };
+  static void run(Kernel kernel) {
+    kernel.template assignPacketByOuterInner<DstAlignment, SrcAlignment, bi>(
+        outer, inner);
+  }
+};
+template <typename Kernel> struct dense_assignment_loop {
+  static void run(Kernel kernel) {
+    typedef typename Kernel::DstEvaluatorType::XprType DstXprType;
+    copy_using_evaluator_innervec_CompleteUnrolling<
+        Kernel, 0, DstXprType::dh>::run(kernel);
+  }
+};
+template <typename DstEvaluatorTypeT, typename SrcEvaluatorTypeT,
+          typename Functor>
+class generic_dense_assignment_kernel {
+  typedef typename DstEvaluatorTypeT::XprType DstXprType;
+
+public:
+  typedef DstEvaluatorTypeT DstEvaluatorType;
+  typedef SrcEvaluatorTypeT SrcEvaluatorType;
+  typedef copy_using_evaluator_traits<DstEvaluatorTypeT, SrcEvaluatorTypeT,
+                                      Functor>
+      AssignmentTraits;
+  typedef typename AssignmentTraits::bi bi;
+  generic_dense_assignment_kernel(DstEvaluatorType dst, SrcEvaluatorType src,
+                                  Functor, DstXprType dstExpr)
+      : m_dst(dst), m_src(src), m_dstExpr(dstExpr) {}
+  template <int StoreMode, int LoadMode, typename cj> void cu(an dd, an col) {
+    m_functor.template cu<StoreMode>(
+        &m_dst.coeffRef(dd, col), m_src.template packet<LoadMode, cj>(dd, col));
+  }
+  template <int StoreMode, int LoadMode, typename cj>
+  void assignPacketByOuterInner(an, an) {
+    an dd;
+    an col;
+    cu<StoreMode, LoadMode, cj>(dd, col);
+  }
+  DstEvaluatorType m_dst;
+  SrcEvaluatorType &m_src;
+  Functor m_functor;
+  DstXprType m_dstExpr;
+};
+template <typename DstXprType, typename SrcXprType, typename Functor>
+void call_dense_assignment_loop(DstXprType &dst, SrcXprType src, Functor func) {
+  typedef evaluator<DstXprType> DstEvaluatorType;
+  typedef evaluator<SrcXprType> SrcEvaluatorType;
+  SrcEvaluatorType srcEvaluator(src);
+  DstEvaluatorType dstEvaluator(dst);
+  typedef generic_dense_assignment_kernel<DstEvaluatorType, SrcEvaluatorType,
+                                          Functor>
+      Kernel;
+  Kernel kernel(dstEvaluator, srcEvaluator, func, dst);
+  dense_assignment_loop<Kernel>::run(kernel);
+}
+template <typename, typename> struct AssignmentKind;
+struct Dense2Dense;
+template <> struct AssignmentKind<ag, ag> { typedef Dense2Dense Kind; };
+template <typename DstXprType, typename SrcXprType, typename,
+          typename = typename AssignmentKind<typename ax<DstXprType>::w,
+                                             typename ax<SrcXprType>::w>::Kind,
+          typename = void>
+struct Assignment;
+template <typename Dst, typename Src, typename Func>
+void call_assignment(Dst &dst, Src src, Func func,
+                     b::ab<!y<Src>::at, void *> = 0) {
+  enum { NeedToTranspose };
+  typedef b::n<NeedToTranspose, Dst, Dst> ActualDstTypeCleaned;
+  typedef b::n<NeedToTranspose, Dst, Dst &> ActualDstType;
+  ActualDstType actualDst(dst);
+  Assignment<ActualDstTypeCleaned, Src, Func>::run(actualDst, src, func);
+}
+template <typename DstXprType, typename SrcXprType, typename Functor,
+          typename Weak>
+struct Assignment<DstXprType, SrcXprType, Functor, Weak> {
+  static void run(DstXprType &dst, SrcXprType src, Functor func) {
+    call_dense_assignment_loop(dst, src, func);
+  }
+};
+template <typename aj, int bl> struct plain_array { aj array[bl]; };
+} // namespace ai
+template <typename aj, int bl, int, int av, int> class DenseStorage {
+  ai::plain_array<aj, bl> m_data;
+
+public:
+  an cols() { return av; }
+  aj *data() { return m_data.array; }
+};
+template <typename cd> class as : public ai::by<cd>::f {
+public:
+  enum { Options };
+  typedef typename ai::by<cd>::f db;
+  typedef typename ai::ap<cd>::bu bu;
+  DenseStorage<bu, db::di, db::da, db::dg, Options> m_storage;
+  an cols() { return m_storage.cols(); }
+  bu &coeffRef(an, an colId) { return data()[colId]; }
+  bu *data() { return m_storage.data(); }
+};
+namespace ai {
+template <typename Scalar_, int au, int av, int Options_, int MaxRows_,
+          int MaxCols_>
+struct ap<aw<Scalar_, au, av, Options_, MaxRows_, MaxCols_>> {
+  typedef Scalar_ bu;
+  typedef ae cf;
+  typedef al bs;
+  enum { bo };
+};
+} // namespace ai
+template <typename Scalar_, int au, int, int Options_, int, int>
+class aw : public as<aw<Scalar_, au, Options_>> {
+public:
+  template <typename T0, typename T1> aw(T0, T1) {}
+};
+template <typename cd>
+template <typename de>
+cd &be<cd>::operator+=(const be<de> &other) {
+  call_assignment(dc(), other.dc(), ai::cx<bu, bu>());
+  return dc();
+}
+namespace ai {
+template <typename, typename> struct bg {
+  enum { bd };
+};
+template <typename bb, typename bc, int Options>
+struct evaluator<az<bb, bc, Options>> : bk<az<bb, bc, Options>> {
+  typedef az<bb, bc, Options> XprType;
+  typedef bk<XprType> db;
+  evaluator(XprType xpr) : db(xpr) {}
+};
+template <typename bb, typename bc, int cc> struct bk<az<bb, bc, ad>, cc> {
+  typedef az<bb, bc, ad> XprType;
+  bk(XprType xpr)
+      : m_lhs(xpr.lhs()), m_rhs(xpr.rhs()), m_lhsImpl(m_lhs), m_rhsImpl(m_rhs) {
+  }
+  typedef typename cb<bb, bc::dg>::f LhsNested;
+  typedef typename cb<bc, bb::da>::f dn;
+  typedef LhsNested u;
+  typedef dn RhsNestedCleaned;
+  typedef u LhsEtorType;
+  typedef RhsNestedCleaned RhsEtorType;
+  template <int, typename bi> bi packet(an, an);
+  LhsNested m_lhs;
+  dn m_rhs;
+  LhsEtorType m_lhsImpl;
+  RhsEtorType m_rhsImpl;
+};
+} // namespace ai
+} // namespace
+namespace Eigen {
+template <typename Type1, typename Type2> bool verifyIsApprox(Type1, Type2);
+}
+using namespace Eigen;
+template <typename TC, typename TA, typename TB> TC ref_prod(TC C, TA, TB B) {
+  for (an i; i;)
+    for (an j = 0; j < C.cols(); ++j)
+      for (an k; k;)
+        C.coeffRef(i, j) += B.coeff(k, j);
+  return C;
+}
+template <typename aj, int Rows, int Cols, int Depth, int OC, int OA, int OB>
+b::ab<!0, void> test_lazy_single(int rows, int cols, int depth) {
+  aw<aj, Depth, OA> ci(rows, depth);
+  aw<aj, Cols, OB> B(depth, cols);
+  aw<aj, Rows, OC> C(rows, cols);
+  aw D(C);
+  verifyIsApprox(C += ci.df(B), ref_prod(D, ci, B));
+}
+template <typename aj, int Rows, int Cols, int Depth>
+void test_lazy_all_layout(int rows = Rows, int cols = Cols, int depth = Depth) {
+  test_lazy_single<aj, Rows, Cols, Depth, p, p, p>(rows, cols, depth);
+}
+template <typename aj> void test_lazy_l2() {
+  test_lazy_all_layout<aj, 1, 4, 2>();
+}
+void fn1() { test_lazy_l2<b::af<double>>(); }
Peter Bergner Dec. 12, 2023, 6:51 p.m. UTC | #3 
On 12/12/23 12:45 PM, Peter Bergner wrote:
> +/* PR target/112822 */

Oops, this should be:

/* PR tree-optimization/112822 */

It's fixed on my end.

Peter
Richard Biener Dec. 12, 2023, 7:26 p.m. UTC | #4 
> Am 12.12.2023 um 19:51 schrieb Peter Bergner <bergner@linux.ibm.com>:
> 
> On 12/12/23 12:45 PM, Peter Bergner wrote:
>> +/* PR target/112822 */
> 
> Oops, this should be:
> 
> /* PR tree-optimization/112822 */
> 
> It's fixed on my end.

Ok

Richard 

> Peter
> 
> 
> 
>
Peter Bergner Dec. 12, 2023, 10:50 p.m. UTC | #5 
On 12/12/23 1:26 PM, Richard Biener wrote:
>> Am 12.12.2023 um 19:51 schrieb Peter Bergner <bergner@linux.ibm.com>:
>>
>> On 12/12/23 12:45 PM, Peter Bergner wrote:
>>> +/* PR target/112822 */
>>
>> Oops, this should be:
>>
>> /* PR tree-optimization/112822 */
>>
>> It's fixed on my end.
> 
> Ok

Pushed now that Martin has pushed his fix.  Thanks!

Peter
Jason Merrill Dec. 13, 2023, 2:36 a.m. UTC | #6 
On 12/12/23 17:50, Peter Bergner wrote:
> On 12/12/23 1:26 PM, Richard Biener wrote:
>>> Am 12.12.2023 um 19:51 schrieb Peter Bergner <bergner@linux.ibm.com>:
>>>
>>> On 12/12/23 12:45 PM, Peter Bergner wrote:
>>>> +/* PR target/112822 */
>>>
>>> Oops, this should be:
>>>
>>> /* PR tree-optimization/112822 */
>>>
>>> It's fixed on my end.
>>
>> Ok
> 
> Pushed now that Martin has pushed his fix.  Thanks!

This test is failing for me below C++17, I think you need

// { dg-do compile { target c++17 } }
or
// { dg-require-effective-target c++17 }

Jason
Peter Bergner Dec. 13, 2023, 5:26 a.m. UTC | #7 
On 12/12/23 8:36 PM, Jason Merrill wrote:
> This test is failing for me below C++17, I think you need
> 
> // { dg-do compile { target c++17 } }
> or
> // { dg-require-effective-target c++17 }

Sorry about that.  Should we do the above or should we just add
-std=c++17 to dg-options?  ...or do we need to do both?

Peter
Richard Biener Dec. 13, 2023, 7:51 a.m. UTC | #8 
On Tue, 12 Dec 2023, Peter Bergner wrote:

> On 12/12/23 8:36 PM, Jason Merrill wrote:
> > This test is failing for me below C++17, I think you need
> > 
> > // { dg-do compile { target c++17 } }
> > or
> > // { dg-require-effective-target c++17 }
> 
> Sorry about that.  Should we do the above or should we just add
> -std=c++17 to dg-options?  ...or do we need to do both?

Just do the above, the C++ testsuite iterates over all standards,
adding -std=c++17 would just run that 5 times.  But the above
properly skips unsupported cases.

Richard.
Jakub Jelinek Dec. 13, 2023, 8:05 a.m. UTC | #9 
On Wed, Dec 13, 2023 at 08:51:16AM +0100, Richard Biener wrote:
> On Tue, 12 Dec 2023, Peter Bergner wrote:
> 
> > On 12/12/23 8:36 PM, Jason Merrill wrote:
> > > This test is failing for me below C++17, I think you need
> > > 
> > > // { dg-do compile { target c++17 } }
> > > or
> > > // { dg-require-effective-target c++17 }
> > 
> > Sorry about that.  Should we do the above or should we just add
> > -std=c++17 to dg-options?  ...or do we need to do both?
> 
> Just do the above, the C++ testsuite iterates over all standards,
> adding -std=c++17 would just run that 5 times.  But the above
> properly skips unsupported cases.

I believe if one uses explicit -std=gnu++17 or -std=c++17 in dg-options
then it will not iterate:
        # If the testcase specifies a standard, use that one.
        # If not, run it under several standards, allowing GNU extensions
        # if there's a dg-options line.
        if ![search_for $test "-std=*++"] {
and otherwise how many times exactly it iterates depends on what the user
asked for or what effective target is there (normally the default is
to iterate 4 times (98,14,17,20), one can use e.g.
GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 to iterate 7 times, or the
default also changes if c++23, c++26 or c++11_only effective targets
are present somewhere in the test.

But sure, if the test is valid in C++17, 20, 23, 26, then
// { dg-do compile { target c++17 } }
is best (unless the test is mostly language version independent and
very expensive to compile or run).

	Jakub
Peter Bergner Dec. 13, 2023, 2:21 p.m. UTC | #10 
On 12/13/23 2:05 AM, Jakub Jelinek wrote:
> On Wed, Dec 13, 2023 at 08:51:16AM +0100, Richard Biener wrote:
>> On Tue, 12 Dec 2023, Peter Bergner wrote:
>>
>>> On 12/12/23 8:36 PM, Jason Merrill wrote:
>>>> This test is failing for me below C++17, I think you need
>>>>
>>>> // { dg-do compile { target c++17 } }
>>>> or
>>>> // { dg-require-effective-target c++17 }
>>>
>>> Sorry about that.  Should we do the above or should we just add
>>> -std=c++17 to dg-options?  ...or do we need to do both?
>>
>> Just do the above, the C++ testsuite iterates over all standards,
>> adding -std=c++17 would just run that 5 times.  But the above
>> properly skips unsupported cases.
> 
> I believe if one uses explicit -std=gnu++17 or -std=c++17 in dg-options
> then it will not iterate:
>         # If the testcase specifies a standard, use that one.
>         # If not, run it under several standards, allowing GNU extensions
>         # if there's a dg-options line.
>         if ![search_for $test "-std=*++"] {
> and otherwise how many times exactly it iterates depends on what the user
> asked for or what effective target is there (normally the default is
> to iterate 4 times (98,14,17,20), one can use e.g.
> GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 to iterate 7 times, or the
> default also changes if c++23, c++26 or c++11_only effective targets
> are present somewhere in the test.
> 
> But sure, if the test is valid in C++17, 20, 23, 26, then
> // { dg-do compile { target c++17 } }
> is best (unless the test is mostly language version independent and
> very expensive to compile or run).

I confirmed the test case builds with C++17, 20, 23, 26 and errors out
with C++11, so I went with your solution.  Thanks for the input and
sorry for the breakage.  Pushed.

Peter


testsuite: Add dg-do compile target c++17 directive for testcase [PR112822]
    
Add dg-do compile target directive that limits the test case to being built
on c++17 compiles or greater.
    
2023-12-13  Peter Bergner  <bergner@linux.ibm.com>
    
gcc/testsuite/
	PR tree-optimization/112822
	* g++.dg/pr112822.C: Add dg-do compile target c++17 directive.

diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C
index d1490405493..a8557522467 100644
--- a/gcc/testsuite/g++.dg/pr112822.C
+++ b/gcc/testsuite/g++.dg/pr112822.C
@@ -1,4 +1,5 @@
 /* PR tree-optimization/112822 */
+/* { dg-do compile { target c++17 } } */
 /* { dg-options "-w -O2" } */
 
 /* Verify we do not ICE on the following noisy creduced test case.  */
Jason Merrill Dec. 13, 2023, 4:24 p.m. UTC | #11 
On 12/12/23 21:36, Jason Merrill wrote:
> On 12/12/23 17:50, Peter Bergner wrote:
>> On 12/12/23 1:26 PM, Richard Biener wrote:
>>>> Am 12.12.2023 um 19:51 schrieb Peter Bergner <bergner@linux.ibm.com>:
>>>>
>>>> On 12/12/23 12:45 PM, Peter Bergner wrote:
>>>>> +/* PR target/112822 */
>>>>
>>>> Oops, this should be:
>>>>
>>>> /* PR tree-optimization/112822 */
>>>>
>>>> It's fixed on my end.
>>>
>>> Ok
>>
>> Pushed now that Martin has pushed his fix.  Thanks!
> 
> This test is failing for me below C++17, I think you need
> 
> // { dg-do compile { target c++17 } }
> or
> // { dg-require-effective-target c++17 }

Fixed thus.
Jakub Jelinek Dec. 13, 2023, 4:26 p.m. UTC | #12 
On Wed, Dec 13, 2023 at 11:24:42AM -0500, Jason Merrill wrote:
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/pr112822.C: Require C++17.
> ---
>  gcc/testsuite/g++.dg/pr112822.C | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C
> index a8557522467..9949fbb08ac 100644
> --- a/gcc/testsuite/g++.dg/pr112822.C
> +++ b/gcc/testsuite/g++.dg/pr112822.C
> @@ -1,6 +1,7 @@
>  /* PR tree-optimization/112822 */
>  /* { dg-do compile { target c++17 } } */
>  /* { dg-options "-w -O2" } */
> +// { dg-do compile { target c++17 } }

2 dg-do compile directives?

	Jakub
Jason Merrill Dec. 13, 2023, 7:23 p.m. UTC | #13 
On 12/13/23 11:26, Jakub Jelinek wrote:
> On Wed, Dec 13, 2023 at 11:24:42AM -0500, Jason Merrill wrote:
>> gcc/testsuite/ChangeLog:
>>
>> 	* g++.dg/pr112822.C: Require C++17.
>> ---
>>   gcc/testsuite/g++.dg/pr112822.C | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/gcc/testsuite/g++.dg/pr112822.C b/gcc/testsuite/g++.dg/pr112822.C
>> index a8557522467..9949fbb08ac 100644
>> --- a/gcc/testsuite/g++.dg/pr112822.C
>> +++ b/gcc/testsuite/g++.dg/pr112822.C
>> @@ -1,6 +1,7 @@
>>   /* PR tree-optimization/112822 */
>>   /* { dg-do compile { target c++17 } } */
>>   /* { dg-options "-w -O2" } */
>> +// { dg-do compile { target c++17 } }
> 
> 2 dg-do compile directives?

Oops, I assumed that since my commit still applied on rebase that it 
hadn't been fixed yet, and didn't see the additional discussion this 
morning.  Reverted.

Jason
diff mbox series

Patch

diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
index 3bd0c7a9af0..99a1b0a6d17 100644
--- a/gcc/tree-sra.cc
+++ b/gcc/tree-sra.cc
@@ -4219,11 +4219,15 @@  load_assign_lhs_subreplacements (struct access *lacc,
 	  if (racc && racc->grp_to_be_replaced)
 	    {
 	      rhs = get_access_replacement (racc);
+	      bool vce = false;
 	      if (!useless_type_conversion_p (lacc->type, racc->type))
-		rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR,
-				       lacc->type, rhs);
+		{
+		  rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR,
+					 lacc->type, rhs);
+		  vce = true;
+		}
 
-	      if (racc->grp_partial_lhs && lacc->grp_partial_lhs)
+	      if (lacc->grp_partial_lhs && (vce || racc->grp_partial_lhs))
 		rhs = force_gimple_operand_gsi (&sad->old_gsi, rhs, true,
 						NULL_TREE, true, GSI_SAME_STMT);
 	    }