Re: Patch ping

[Segher Boessenkool]

On Mon, Jun 17, 2024 at 03:26:52PM +0200, Jakub Jelinek wrote:
> I'd like to ping the
> https://gcc.gnu.org/pipermail/gcc-patches/2024-June/653573.html
> patch.  While the committed and backported patch fixed PCH on PIE
> cc1/cc1plus etc. on PowerPC, it grew up the size of the
> rs6000_init_generated_builtins function quite a lot.
> The above patch decreases it back, to even less than the size of
> the function before my fix.

A patch in the middle of a thread.  I missed it, sorry.  Please send
patches as separate threads?


Segher

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jan Hubicka]

> > +  if (POINTER_TYPE_P (TREE_TYPE (t1)))
> > +{
> > +  if (SSA_NAME_PTR_INFO (t1))
> > +   {
> > + if (!SSA_NAME_PTR_INFO (t2)
> > + || SSA_NAME_PTR_INFO (t1)->align != SSA_NAME_PTR_INFO (t2)->align
> > + || SSA_NAME_PTR_INFO (t1)->misalign != SSA_NAME_PTR_INFO 
> > (t2)->misalign)
> 
> You want to compare SSA_NAME_PTR_INFO ()->pt.zero as well I think since
> we store pointer non-null-ness from VRP there.
> 
> You are not comparing the actual points-to info - but of course I'd
> expect that to differ as soon as local decls are involved.  Still looks
> like a hole to me.

I convinced myself that we don't need to do that since we recompute PTA
after IPA stage: unlike value ranges it is thrown away and recomputed
rather then just refined from prevoius solution.  But indeed if parts
are persistent, we need to compare it (and should stream it to LTO I
guess).

Honza

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Richard Biener]

On Thu, 14 Mar 2024, Jan Hubicka wrote:

> > > We have wrong code with LTO, too.
> > 
> > I know.
> > 
> > > The problem is that IPA passes (and
> > > not only that, loop analysis too) does analysis at compile time (with
> > > value numbers in) and streams the info separately.
> > 
> > And that is desirable, because otherwise it simply couldn't derive any
> > ranges.
> > 
> > >  Removal of value ranges
> > > (either by LTO or by your patch) happens between computing these
> > > summaries and using them, so this can be used to trigger wrong code,
> > > sadly.
> > 
> > Yes.  But with LTO, I don't see how the IPA ICF comparison whether
> > two functions are the same or not could be done with the
> > SSA_NAME_{RANGE,PTR}_INFO in, otherwise it could only ICF merge functions
> > from the same TUs.  So the comparison IMHO (and the assert checks in my
> > patch prove that) is done when the SSA_NAME_{RANGE,PTR}_INFO aren't in
> > anymore.  So, one just needs to compare and punt or union whatever
> > is or could be influenced in the IPA streamed data from the ranges etc.
> > And because one has to do it for LTO, doing it for non-LTO should be
> > sufficient too.
> 
> Sorry, this was bit of a misunderstanding: I tought you still considered
> the original patch to be full fix, while I tought I should look into it
> more and dig out more issues.  This is bit of can of worms.  Overall I
> think the plan is:
> 
> This stage4
> 1) fix VR divergences by either comparing or droping them
> 2) fix jump function differences by comparing them
>(I just constructed a testcase showing that jump functions can
> diverge for other reasons than just VR, so this is deeper problem,
> too)
> 3) try to construct aditional wrong code testcases (finite_p
>divergences, trapping)
> Next stage1
> 4) implement VR and PTR info streaming
> 5) make ICF to compare VRs and punt otherwise
> 6) implement optimize_size feature to ICF that will not punt on
>diverging TBAA or value ranges and do merging instead.
>We need some extra infrastructure for that, being able to keep the
>maps between basic blocks and SSA names from comparsion stage to
>merging stage
> 7) measure how effective ICF becomes in optimize_size mode and implement
>heuristics on how much metadata merging we want to do with -O2/-O3 as
>well.
>Based on older data Martin collected for his thesis, ICF used to be
>must more effective on libreoffice then it is today, so hopefully we
>can recover 10-15% binary size improvmeents here.
> 8) General ICF TLC.  There are many half finished things for a while in
>this pass (such as merging with different BB or stmt orders).
> 
> I am attaching the compare patch which also fixes the original wrong
> code. If you preffer your version, just go ahead to commit it. Otherwise
> I will add your testcase for this patch and commit this one.
> Statistically we almost never merge functions with different value
> ranges (three in testsuite, 0 during bootstrap, 1 during LTO bootstrap
> and probably few in LLVM build - there are 15 cases reported, but some
> are false positives caused by hash function conflicts).
> 
> Honza
> 
> gcc/ChangeLog:
> 
>   * ipa-icf-gimple.cc (func_checker::compare_ssa_name): Compare value 
> ranges.
> 
> diff --git a/gcc/ipa-icf-gimple.cc b/gcc/ipa-icf-gimple.cc
> index 8c2df7a354e..37c416d777d 100644
> --- a/gcc/ipa-icf-gimple.cc
> +++ b/gcc/ipa-icf-gimple.cc
> @@ -39,9 +39,11 @@ along with GCC; see the file COPYING3.  If not see
>  #include "cfgloop.h"
>  #include "attribs.h"
>  #include "gimple-walk.h"
> +#include "value-query.h"
> +#include "value-range-storage.h"
>  
>  #include "tree-ssa-alias-compare.h"
>  #include "ipa-icf-gimple.h"
>  
>  namespace ipa_icf_gimple {
>  
> @@ -109,6 +116,35 @@ func_checker::compare_ssa_name (const_tree t1, 
> const_tree t2)
>else if (m_target_ssa_names[i2] != (int) i1)
>  return false;
>  
> +  if (POINTER_TYPE_P (TREE_TYPE (t1)))
> +{
> +  if (SSA_NAME_PTR_INFO (t1))
> + {
> +   if (!SSA_NAME_PTR_INFO (t2)
> +   || SSA_NAME_PTR_INFO (t1)->align != SSA_NAME_PTR_INFO (t2)->align
> +   || SSA_NAME_PTR_INFO (t1)->misalign != SSA_NAME_PTR_INFO 
> (t2)->misalign)

You want to compare SSA_NAME_PTR_INFO ()->pt.zero as well I think since
we store pointer non-null-ness from VRP there.

You are not comparing the actual points-to info - but of course I'd
expect that to differ as soon as local decls are involved.  Still looks
like a hole to me.

> + return false;
> + }
> +  else if (SSA_NAME_PTR_INFO (t2))
> + return false;
> +}
> +  else
> +{
> +  if (SSA_NAME_RANGE_INFO (t1))
> + {
> +   if (!SSA_NAME_RANGE_INFO (t2))
> + return false;
> +   Value_Range r1 (TREE_TYPE (t1));
> +   Value_Range r2 (TREE_TYPE (t2));
> +   SSA_NAME_RANGE_INFO (t1)->get_vrange (r1, TREE_TYPE (t1));
> +   SSA_NAME_RANGE_INFO (t2)->get_vrange (r2, TREE_TYPE (t2));
> 

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jan Hubicka]

> > Otherwise
> > I will add your testcase for this patch and commit this one.
> > Statistically we almost never merge functions with different value
> > ranges (three in testsuite, 0 during bootstrap, 1 during LTO bootstrap
> > and probably few in LLVM build - there are 15 cases reported, but some
> > are false positives caused by hash function conflicts).
> 
> It is mostly the fnsplit functions, sure, because we don't really drop
> the __builtin_unreachable checks before IPA and so the if (cond)
> __builtin_unreachable (); style assertions or [[assume (cond)]]; still
> result in ICF failures.

Actually on llvm they are not split functions, but functions where value
ranges were determined by early VRP based on code optimized out by time
we reach ipa-icf (the equal thingy from libstdc++ I wrote about in
previous email)

Honza

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jakub Jelinek]

On Thu, Mar 14, 2024 at 05:16:59PM +0100, Jan Hubicka wrote:
> Sorry, this was bit of a misunderstanding: I tought you still considered
> the original patch to be full fix, while I tought I should look into it
> more and dig out more issues.  This is bit of can of worms.  Overall I
> think the plan is:
> 
> This stage4
> 1) fix VR divergences by either comparing or droping them
> 2) fix jump function differences by comparing them
>(I just constructed a testcase showing that jump functions can
> diverge for other reasons than just VR, so this is deeper problem,
> too)
> 3) try to construct aditional wrong code testcases (finite_p
>divergences, trapping)
> Next stage1
> 4) implement VR and PTR info streaming
> 5) make ICF to compare VRs and punt otherwise
> 6) implement optimize_size feature to ICF that will not punt on
>diverging TBAA or value ranges and do merging instead.
>We need some extra infrastructure for that, being able to keep the
>maps between basic blocks and SSA names from comparsion stage to
>merging stage
> 7) measure how effective ICF becomes in optimize_size mode and implement
>heuristics on how much metadata merging we want to do with -O2/-O3 as
>well.
>Based on older data Martin collected for his thesis, ICF used to be
>must more effective on libreoffice then it is today, so hopefully we
>can recover 10-15% binary size improvmeents here.
> 8) General ICF TLC.  There are many half finished things for a while in
>this pass (such as merging with different BB or stmt orders).

Agreed.

> I am attaching the compare patch which also fixes the original wrong
> code. If you preffer your version, just go ahead to commit it.

Seems both patches are the same size (at least when looking at number of
added lines).  I think I prefer my patch because it will make the LTO and
non-LTO cases more similar which IMHO helps maintainance of the release
branches.  So, e.g. for all the other wrong-code issues
like https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113907#c54
or https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113907#c58 one can actually
trigger them with both non-LTO and LTO, rather than only with LTO and for
non-LTO not trigger because there are SSA_NAME_RANGE_INFO differences that
prevent ICF.
If we start streaming SSA_NAME_RANGE_INFO in GCC 15, that changes things
and then we can decide what to do for differences, 5) or 6) or combinations
thereof (e.g. consider not just if the ranges are different, but how much
too or if one is a subset or superset of the other to decide between punting
and unioning for non-Os).

> Otherwise
> I will add your testcase for this patch and commit this one.
> Statistically we almost never merge functions with different value
> ranges (three in testsuite, 0 during bootstrap, 1 during LTO bootstrap
> and probably few in LLVM build - there are 15 cases reported, but some
> are false positives caused by hash function conflicts).

It is mostly the fnsplit functions, sure, because we don't really drop
the __builtin_unreachable checks before IPA and so the if (cond)
__builtin_unreachable (); style assertions or [[assume (cond)]]; still
result in ICF failures.

Jakub

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jan Hubicka]

> > We have wrong code with LTO, too.
> 
> I know.
> 
> > The problem is that IPA passes (and
> > not only that, loop analysis too) does analysis at compile time (with
> > value numbers in) and streams the info separately.
> 
> And that is desirable, because otherwise it simply couldn't derive any
> ranges.
> 
> >  Removal of value ranges
> > (either by LTO or by your patch) happens between computing these
> > summaries and using them, so this can be used to trigger wrong code,
> > sadly.
> 
> Yes.  But with LTO, I don't see how the IPA ICF comparison whether
> two functions are the same or not could be done with the
> SSA_NAME_{RANGE,PTR}_INFO in, otherwise it could only ICF merge functions
> from the same TUs.  So the comparison IMHO (and the assert checks in my
> patch prove that) is done when the SSA_NAME_{RANGE,PTR}_INFO aren't in
> anymore.  So, one just needs to compare and punt or union whatever
> is or could be influenced in the IPA streamed data from the ranges etc.
> And because one has to do it for LTO, doing it for non-LTO should be
> sufficient too.

Sorry, this was bit of a misunderstanding: I tought you still considered
the original patch to be full fix, while I tought I should look into it
more and dig out more issues.  This is bit of can of worms.  Overall I
think the plan is:

This stage4
1) fix VR divergences by either comparing or droping them
2) fix jump function differences by comparing them
   (I just constructed a testcase showing that jump functions can
diverge for other reasons than just VR, so this is deeper problem,
too)
3) try to construct aditional wrong code testcases (finite_p
   divergences, trapping)
Next stage1
4) implement VR and PTR info streaming
5) make ICF to compare VRs and punt otherwise
6) implement optimize_size feature to ICF that will not punt on
   diverging TBAA or value ranges and do merging instead.
   We need some extra infrastructure for that, being able to keep the
   maps between basic blocks and SSA names from comparsion stage to
   merging stage
7) measure how effective ICF becomes in optimize_size mode and implement
   heuristics on how much metadata merging we want to do with -O2/-O3 as
   well.
   Based on older data Martin collected for his thesis, ICF used to be
   must more effective on libreoffice then it is today, so hopefully we
   can recover 10-15% binary size improvmeents here.
8) General ICF TLC.  There are many half finished things for a while in
   this pass (such as merging with different BB or stmt orders).

I am attaching the compare patch which also fixes the original wrong
code. If you preffer your version, just go ahead to commit it. Otherwise
I will add your testcase for this patch and commit this one.
Statistically we almost never merge functions with different value
ranges (three in testsuite, 0 during bootstrap, 1 during LTO bootstrap
and probably few in LLVM build - there are 15 cases reported, but some
are false positives caused by hash function conflicts).

Honza

gcc/ChangeLog:

* ipa-icf-gimple.cc (func_checker::compare_ssa_name): Compare value 
ranges.

diff --git a/gcc/ipa-icf-gimple.cc b/gcc/ipa-icf-gimple.cc
index 8c2df7a354e..37c416d777d 100644
--- a/gcc/ipa-icf-gimple.cc
+++ b/gcc/ipa-icf-gimple.cc
@@ -39,9 +39,11 @@ along with GCC; see the file COPYING3.  If not see
 #include "cfgloop.h"
 #include "attribs.h"
 #include "gimple-walk.h"
+#include "value-query.h"
+#include "value-range-storage.h"
 
 #include "tree-ssa-alias-compare.h"
 #include "ipa-icf-gimple.h"
 
 namespace ipa_icf_gimple {
 
@@ -109,6 +116,35 @@ func_checker::compare_ssa_name (const_tree t1, const_tree 
t2)
   else if (m_target_ssa_names[i2] != (int) i1)
 return false;
 
+  if (POINTER_TYPE_P (TREE_TYPE (t1)))
+{
+  if (SSA_NAME_PTR_INFO (t1))
+   {
+ if (!SSA_NAME_PTR_INFO (t2)
+ || SSA_NAME_PTR_INFO (t1)->align != SSA_NAME_PTR_INFO (t2)->align
+ || SSA_NAME_PTR_INFO (t1)->misalign != SSA_NAME_PTR_INFO 
(t2)->misalign)
+   return false;
+   }
+  else if (SSA_NAME_PTR_INFO (t2))
+   return false;
+}
+  else
+{
+  if (SSA_NAME_RANGE_INFO (t1))
+   {
+ if (!SSA_NAME_RANGE_INFO (t2))
+   return false;
+ Value_Range r1 (TREE_TYPE (t1));
+ Value_Range r2 (TREE_TYPE (t2));
+ SSA_NAME_RANGE_INFO (t1)->get_vrange (r1, TREE_TYPE (t1));
+ SSA_NAME_RANGE_INFO (t2)->get_vrange (r2, TREE_TYPE (t2));
+ if (r1 != r2)
+   return false;
+   }
+  else if (SSA_NAME_RANGE_INFO (t2))
+   return false;
+}
+
   if (SSA_NAME_IS_DEFAULT_DEF (t1))
 {
   tree b1 = SSA_NAME_VAR (t1);

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jan Hubicka]

> > int test (int a)
> > {
> > return a>0 ? CST1:  CST2;
> > }
> > 
> > gets same hash value no matter what CST1/CST2 is.  I added hasher and I
> > am re-running stats.
> 
> The hash should be commutative here at least.

It needs to match what comparator is doing later, and sadly it does not
try to find the right order of basic blocks.  I added TODO and will fix
it next stage1.

With the attached patch we have no comparsion with mismatching VRP
ranges with non-LTO bootstrap and there are 4 instances of mismatched
jump function with LTO bootstrap.

I checked them and these are functions that are still different and 
would not be unified anyway.  In testsuite we have
 gcc.c-torture/execute/builtin-prefetch-4.c
   this matches
   [irange] long unsigned int [0, 2147483647][18446744071562067968, +INF]
   [irange] long unsigned int [0, 2147483646][18446744071562067968, +INF]
   in postdec_glob_idx/22new vs predec_glob_idx/20

   The functions are different, but have precisely same statements, just
   differ by SSA names that we do not hash.
 c-c++-common/builtins.c
   [irange] long unsigned int [2, 2147483647] MASK 0x7ffe VALUE 0x0
   [irange] long unsigned int [8, 2147483647] MASK 0x7ff8 VALUE 0x0
   in bar.part.0/10new  foo.part.0/9

   This is a real mismatch
 23_containers/vector/cons/2.cc
   [irange] long unsigned int [1, 9223372036854775807] MASK 0x7fff 
VALUE 0x0
   [irange] long unsigned int [1, 9223372036854775807] MASK 0xfff8 
VALUE 0x0
   static std::vector<_Tp, _Alloc>::pointer std::vector<_Tp, 
_Alloc>::_S_do_relocate(pointer, pointer, pointer, _Tp_alloc_type&, 
std::true_type) [with _Tp = A; _Alloc = std::allocator >]/3482
   static std::vector<_Tp, _Alloc>::pointer std::vector<_Tp, 
_Alloc>::_S_do_relocate(pointer, pointer, pointer, _Tp_alloc_type&, 
std::true_type) [with _Tp = double; _Alloc = std::allocator]/3372

   Here funtions are different initially but are optimized to same body
   while we keep info about ranges from optimized out code.

   I tried to make easy testcase, but

   int a;
   a = (char)a;

   is not optimized away, I wonder why, when for all valid values this
   is noop?

 25_algorithms/lexicographical_compare/uchar.cc
   [irange] long unsigned int [8, +INF] MASK 0xfff8 VALUE 0x0
   [irange] long unsigned int [4, +INF] MASK 0xfffc VALUE 0x0
   static constexpr bool std::__equal::equal(const _Tp*, const _Tp*, 
const _Tp*) [with _Tp = long int]/13669
   static constexpr bool std::__equal::equal(const _Tp*, const _Tp*, 
const _Tp*) [with _Tp = int]/13663
 std/ranges/conv/1.cc
   [irange] long unsigned int [8, +INF] MASK 0xfff8 VALUE 0x0
   [irange] long unsigned int [4, +INF] MASK 0xfffc VALUE 0x0
   static constexpr bool std::__equal::equal(const _Tp*, const _Tp*, 
const _Tp*) [with _Tp = long int]/13757
   static constexpr bool std::__equal::equal(const _Tp*, const _Tp*, 
const _Tp*) [with _Tp = int]/13751

   Those two are also happening in llvm.  We try to merge two functions
   which take pointer parameters of different type.

boolD.2783 std::__equal::equalD.426577 (const intD.9 * 
__first1D.433380, const intD.9 * __last1D.433381, const intD.9 * 
__first2D.433382)
{ 
  const intD.9 * __first1_4(D) = __first1D.433380;
  const intD.9 * __last1_3(D) = __last1D.433381;
  const intD.9 * __first2_6(D) = __first2D.433382;
  long intD.12 _1;
  boolD.2783 _2;
  long unsigned intD.16 _7;
  boolD.2783 _8;
  intD.9 _9;

  _1 = __last1_3(D) - __first1_4(D);
  if (_1 != 0)
goto ; [50.00%]
  else
goto ; [50.00%]

  # RANGE [irange] long unsigned int [4, +INF] MASK 0xfffc 
VALUE 0x0
  _7 = (long unsigned intD.16) _1;

  Compared to

boolD.2783 std::__equal::equalD.426685 (const long 
intD.12 * __first1D.433472, const long intD.12 * __last1D.433473, const long 
intD.12 * __first2D.433474)
{
  const long intD.12 * __first1_4(D) = __first1D.433472;
  const long intD.12 * __last1_3(D) = __last1D.433473;
  const long intD.12 * __first2_6(D) = __first2D.433474;
  long intD.12 _1;
  boolD.2783 _2;
  long unsigned intD.16 _7;
  boolD.2783 _8;
  intD.9 _9;

  _1 = __last1_3(D) - __first1_4(D);
  if (_1 != 0)
goto ; [50.00%]
  else
goto ; [50.00%]

  # RANGE [irange] long unsigned int [8, +INF] MASK 0xfff8 
VALUE 0x0
  _7 = (long unsigned intD.16) _1;

   This looks like potentially dangerous situation, since if we can
   derive range from pointed-to type, then ICF needs to match them too.

   However with
   #include 
   size_t diff (long *a, long *b)
   {
 long int d = (b - a) * sizeof (long);
 if (!d)
   abort ();
 return d;
   }
   size_t diff2 (int *a, int *b)
   {
 long int d = (b - a) * 

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jakub Jelinek]

On Wed, Mar 13, 2024 at 12:18:45PM +0100, Jan Hubicka wrote:
> > On Wed, Mar 13, 2024 at 10:55:07AM +0100, Jan Hubicka wrote:
> > > > > So the ipa_jump_func are I think the only thing that actually can 
> > > > > differ
> > > > > on the ICF merging candidates from value range POV.
> > > > 
> > > > I agree.  Btw, I would have approved the original patch in this
> > > > thread that wipes SSA_NAME_INFO in merged bodies to mimic what LTO
> > > > effectively does right now.  That also looks most sensible to
> > > > backport.
> > > > 
> > > > But I'll defer to Honza in the end (but also want to point out we
> > > > need something suitable for backporting).
> > > 
> > > My main worry here is that I tried to relax matching of IL metadata in
> > > the past and it triggers interesting problems.  (I implemented TBAA
> > > merging and one needs to match additional things in summaries and loop
> > > structures)
> > 
> > The point of the patch is that it emulates what happens with LTO (though,
> > does that only for successful ICF merges), because LTO streaming ignores
> > SSA_NAME_{RANGE,PTR}_INFO.
> > So, because with LTO all you have in the IL is the m_vr in jump_tables,
> > pure/const analysis results, whatever else might be derived on the side
> > from the range info, you need to punt or union all that information anyway,
> > otherwise it will misbehave with LTO.
> > So punting on SSA_NAME_{RANGE,PTR}_INFO differences instead of throwing it
> > away means that non-LTO will get fewer ICF merges than LTO unnecessarily,
> > it doesn't improve anything for the code correctness at least for the LTO
> > case.
> 
> We have wrong code with LTO, too.

I know.

> The problem is that IPA passes (and
> not only that, loop analysis too) does analysis at compile time (with
> value numbers in) and streams the info separately.

And that is desirable, because otherwise it simply couldn't derive any
ranges.

>  Removal of value ranges
> (either by LTO or by your patch) happens between computing these
> summaries and using them, so this can be used to trigger wrong code,
> sadly.

Yes.  But with LTO, I don't see how the IPA ICF comparison whether
two functions are the same or not could be done with the
SSA_NAME_{RANGE,PTR}_INFO in, otherwise it could only ICF merge functions
from the same TUs.  So the comparison IMHO (and the assert checks in my
patch prove that) is done when the SSA_NAME_{RANGE,PTR}_INFO aren't in
anymore.  So, one just needs to compare and punt or union whatever
is or could be influenced in the IPA streamed data from the ranges etc.
And because one has to do it for LTO, doing it for non-LTO should be
sufficient too.

Jakub

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jan Hubicka]

> On Wed, Mar 13, 2024 at 10:55:07AM +0100, Jan Hubicka wrote:
> > > > So the ipa_jump_func are I think the only thing that actually can differ
> > > > on the ICF merging candidates from value range POV.
> > > 
> > > I agree.  Btw, I would have approved the original patch in this
> > > thread that wipes SSA_NAME_INFO in merged bodies to mimic what LTO
> > > effectively does right now.  That also looks most sensible to
> > > backport.
> > > 
> > > But I'll defer to Honza in the end (but also want to point out we
> > > need something suitable for backporting).
> > 
> > My main worry here is that I tried to relax matching of IL metadata in
> > the past and it triggers interesting problems.  (I implemented TBAA
> > merging and one needs to match additional things in summaries and loop
> > structures)
> 
> The point of the patch is that it emulates what happens with LTO (though,
> does that only for successful ICF merges), because LTO streaming ignores
> SSA_NAME_{RANGE,PTR}_INFO.
> So, because with LTO all you have in the IL is the m_vr in jump_tables,
> pure/const analysis results, whatever else might be derived on the side
> from the range info, you need to punt or union all that information anyway,
> otherwise it will misbehave with LTO.
> So punting on SSA_NAME_{RANGE,PTR}_INFO differences instead of throwing it
> away means that non-LTO will get fewer ICF merges than LTO unnecessarily,
> it doesn't improve anything for the code correctness at least for the LTO
> case.

We have wrong code with LTO, too.  The problem is that IPA passes (and
not only that, loop analysis too) does analysis at compile time (with
value numbers in) and streams the info separately.  Removal of value ranges
(either by LTO or by your patch) happens between computing these
summaries and using them, so this can be used to trigger wrong code,
sadly.

Honza
> 
>   Jakub
> 

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jakub Jelinek]

On Wed, Mar 13, 2024 at 10:55:07AM +0100, Jan Hubicka wrote:
> > > So the ipa_jump_func are I think the only thing that actually can differ
> > > on the ICF merging candidates from value range POV.
> > 
> > I agree.  Btw, I would have approved the original patch in this
> > thread that wipes SSA_NAME_INFO in merged bodies to mimic what LTO
> > effectively does right now.  That also looks most sensible to
> > backport.
> > 
> > But I'll defer to Honza in the end (but also want to point out we
> > need something suitable for backporting).
> 
> My main worry here is that I tried to relax matching of IL metadata in
> the past and it triggers interesting problems.  (I implemented TBAA
> merging and one needs to match additional things in summaries and loop
> structures)

The point of the patch is that it emulates what happens with LTO (though,
does that only for successful ICF merges), because LTO streaming ignores
SSA_NAME_{RANGE,PTR}_INFO.
So, because with LTO all you have in the IL is the m_vr in jump_tables,
pure/const analysis results, whatever else might be derived on the side
from the range info, you need to punt or union all that information anyway,
otherwise it will misbehave with LTO.
So punting on SSA_NAME_{RANGE,PTR}_INFO differences instead of throwing it
away means that non-LTO will get fewer ICF merges than LTO unnecessarily,
it doesn't improve anything for the code correctness at least for the LTO
case.

Jakub

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Richard Biener]

On Wed, 13 Mar 2024, Jan Hubicka wrote:

> > On Tue, 12 Mar 2024, Jakub Jelinek wrote:
> > 
> > > On Tue, Mar 12, 2024 at 05:21:58PM +0100, Jakub Jelinek wrote:
> > > > On Tue, Mar 12, 2024 at 10:46:42AM +0100, Jan Hubicka wrote:
> > > > > I am sorry for delaying this.  I made the variant that simply compares
> > > > > value range of functions and prevents merging if they diverge and 
> > > > > wanted
> > > > > to make some bigger statistics.  This made me notice some performance
> > > > > problems on clang performance and libstdc++ RB-trees which disrailed 
> > > > > me
> > > > > from the original PR.  I will finish the statistics today.
> > > > 
> > > > With the posted patch, perhaps if we don't want to union jump_tables 
> > > > etc.,
> > > > all we could punt on is differences in the jump_table VRs rather than 
> > > > just
> > > > any SSA_NAME_RANGE_INFO differences.
> > > 
> > > To expand on this, I think we need to either union or punt on jump_func
> > > differences in any case, because for LTO we can't really punt on
> > > SSA_NAME_RANGE_INFO differences given that we don't stream that out and 
> > > in.
> 
> I noticed that yesterday too (I added my jump function testcase to
> testsuit and it fails with -flto, too).  I implemented comparator for
> them too and run the stats.  There was over 3000 functions in bootstrap
> where we run into differences in value-range and about 150k in LLVM
> build.
> 
> Inspecting random examples shown that those are usually false positives
> (pair of functions that are different but triggers hash colision) caused
> by the fact that we do not hash PHI arguments, so code like
> 
> int test (int a)
> {
> return a>0 ? CST1:  CST2;
> }
> 
> gets same hash value no matter what CST1/CST2 is.  I added hasher and I
> am re-running stats.

The hash should be commutative here at least.

> > > So the ipa_jump_func are I think the only thing that actually can differ
> > > on the ICF merging candidates from value range POV.
> > 
> > I agree.  Btw, I would have approved the original patch in this
> > thread that wipes SSA_NAME_INFO in merged bodies to mimic what LTO
> > effectively does right now.  That also looks most sensible to
> > backport.
> > 
> > But I'll defer to Honza in the end (but also want to point out we
> > need something suitable for backporting).
> 
> My main worry here is that I tried to relax matching of IL metadata in
> the past and it triggers interesting problems.  (I implemented TBAA
> merging and one needs to match additional things in summaries and loop
> structures)
> 
> If value range differs at IPA analysis time, we need to be sure that we
> compare everything that possibly depends on it. So it is always safer to
> just compare more than try to merge. Which is what we do in all cases so
> far.  Here, for the first time, is the problem is with LTO streaming
> missing the data though.
> 
> Thinking more about it, I wonder if different value ranges can be
> exploited to cause different decisions about function being finite
> (confuse pure/const) or different outcome of alias analysis yielding to
> different aggregate jump functions (confusing ipa-prop).

The obvious thing would be range info making it possible to prove
a stmt cannot trap, say for an array index or for arithmetic with
-ftrapv.  But I'm not sure that makes a differnce for pure/const-ness.

Making something looping vs. non-looping should be easy though,
just have range info for a loop with an unsigned IV that evolves
like { 0, +, increment } and with a != exit condition where for some 
'increment' we know we eventually reach equality but with some
other we know we never do.

But that just means pure/const state needs to be recomputed after
merging?  Or compared.

Richard.

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jan Hubicka]

> On Tue, 12 Mar 2024, Jakub Jelinek wrote:
> 
> > On Tue, Mar 12, 2024 at 05:21:58PM +0100, Jakub Jelinek wrote:
> > > On Tue, Mar 12, 2024 at 10:46:42AM +0100, Jan Hubicka wrote:
> > > > I am sorry for delaying this.  I made the variant that simply compares
> > > > value range of functions and prevents merging if they diverge and wanted
> > > > to make some bigger statistics.  This made me notice some performance
> > > > problems on clang performance and libstdc++ RB-trees which disrailed me
> > > > from the original PR.  I will finish the statistics today.
> > > 
> > > With the posted patch, perhaps if we don't want to union jump_tables etc.,
> > > all we could punt on is differences in the jump_table VRs rather than just
> > > any SSA_NAME_RANGE_INFO differences.
> > 
> > To expand on this, I think we need to either union or punt on jump_func
> > differences in any case, because for LTO we can't really punt on
> > SSA_NAME_RANGE_INFO differences given that we don't stream that out and in.

I noticed that yesterday too (I added my jump function testcase to
testsuit and it fails with -flto, too).  I implemented comparator for
them too and run the stats.  There was over 3000 functions in bootstrap
where we run into differences in value-range and about 150k in LLVM
build.

Inspecting random examples shown that those are usually false positives
(pair of functions that are different but triggers hash colision) caused
by the fact that we do not hash PHI arguments, so code like

int test (int a)
{
return a>0 ? CST1:  CST2;
}

gets same hash value no matter what CST1/CST2 is.  I added hasher and I
am re-running stats.

> > So the ipa_jump_func are I think the only thing that actually can differ
> > on the ICF merging candidates from value range POV.
> 
> I agree.  Btw, I would have approved the original patch in this
> thread that wipes SSA_NAME_INFO in merged bodies to mimic what LTO
> effectively does right now.  That also looks most sensible to
> backport.
> 
> But I'll defer to Honza in the end (but also want to point out we
> need something suitable for backporting).

My main worry here is that I tried to relax matching of IL metadata in
the past and it triggers interesting problems.  (I implemented TBAA
merging and one needs to match additional things in summaries and loop
structures)

If value range differs at IPA analysis time, we need to be sure that we
compare everything that possibly depends on it. So it is always safer to
just compare more than try to merge. Which is what we do in all cases so
far.  Here, for the first time, is the problem is with LTO streaming
missing the data though.

Thinking more about it, I wonder if different value ranges can be
exploited to cause different decisions about function being finite
(confuse pure/const) or different outcome of alias analysis yielding to
different aggregate jump functions (confusing ipa-prop).

I will try to build testcases today.
Honza
> 
> Richard.

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Richard Biener]

On Tue, 12 Mar 2024, Jakub Jelinek wrote:

> On Tue, Mar 12, 2024 at 05:21:58PM +0100, Jakub Jelinek wrote:
> > On Tue, Mar 12, 2024 at 10:46:42AM +0100, Jan Hubicka wrote:
> > > I am sorry for delaying this.  I made the variant that simply compares
> > > value range of functions and prevents merging if they diverge and wanted
> > > to make some bigger statistics.  This made me notice some performance
> > > problems on clang performance and libstdc++ RB-trees which disrailed me
> > > from the original PR.  I will finish the statistics today.
> > 
> > With the posted patch, perhaps if we don't want to union jump_tables etc.,
> > all we could punt on is differences in the jump_table VRs rather than just
> > any SSA_NAME_RANGE_INFO differences.
> 
> To expand on this, I think we need to either union or punt on jump_func
> differences in any case, because for LTO we can't really punt on
> SSA_NAME_RANGE_INFO differences given that we don't stream that out and in.
> So the ipa_jump_func are I think the only thing that actually can differ
> on the ICF merging candidates from value range POV.

I agree.  Btw, I would have approved the original patch in this
thread that wipes SSA_NAME_INFO in merged bodies to mimic what LTO
effectively does right now.  That also looks most sensible to
backport.

But I'll defer to Honza in the end (but also want to point out we
need something suitable for backporting).

Richard.

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jakub Jelinek]

On Tue, Mar 12, 2024 at 05:21:58PM +0100, Jakub Jelinek wrote:
> On Tue, Mar 12, 2024 at 10:46:42AM +0100, Jan Hubicka wrote:
> > I am sorry for delaying this.  I made the variant that simply compares
> > value range of functions and prevents merging if they diverge and wanted
> > to make some bigger statistics.  This made me notice some performance
> > problems on clang performance and libstdc++ RB-trees which disrailed me
> > from the original PR.  I will finish the statistics today.
> 
> With the posted patch, perhaps if we don't want to union jump_tables etc.,
> all we could punt on is differences in the jump_table VRs rather than just
> any SSA_NAME_RANGE_INFO differences.

To expand on this, I think we need to either union or punt on jump_func
differences in any case, because for LTO we can't really punt on
SSA_NAME_RANGE_INFO differences given that we don't stream that out and in.
So the ipa_jump_func are I think the only thing that actually can differ
on the ICF merging candidates from value range POV.

Jakub

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jakub Jelinek]

On Tue, Mar 12, 2024 at 10:46:42AM +0100, Jan Hubicka wrote:
> I am sorry for delaying this.  I made the variant that simply compares
> value range of functions and prevents merging if they diverge and wanted
> to make some bigger statistics.  This made me notice some performance
> problems on clang performance and libstdc++ RB-trees which disrailed me
> from the original PR.  I will finish the statistics today.

With the posted patch, perhaps if we don't want to union jump_tables etc.,
all we could punt on is differences in the jump_table VRs rather than just
any SSA_NAME_RANGE_INFO differences.
But I agree that for GCC 15 we really want to merge rather than punt.

> For next stage1 we definitly want to move ahead with merging metadata
> (not only value ranges, but hopefully also TBAA).  Currently the only
> thing we merge aggressively is the edge profile (and we have PR on that
> merging functions which differs only but likely/unlikely attribute).
> However for backportability and for this avoiding merging may be safer
> solution depending on the stats.A
> 
> I was looking into ipa-vrp useability and there are several tests on
> Firefox talos where ipa-prop VRP makes difference. It boils down to
> propagating fact that parameter is alway snon-NULL.  This is commonly
> tested in C++ casts.

Jakub

Re: Patch ping Re: [PATCH] icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

[Jan Hubicka]

> Hi!
Hi,
> 
> On Thu, Feb 15, 2024 at 08:29:24AM +0100, Jakub Jelinek wrote:
> > 2024-02-15  Jakub Jelinek  
> > 
> > PR middle-end/113907
> > * ipa-icf.cc (sem_item_optimizer::merge_classes): Reset
> > SSA_NAME_RANGE_INFO and SSA_NAME_PTR_INFO on successfully ICF merged
> > functions.
> > 
> > * gcc.dg/pr113907.c: New test.
> 
> I'd like to ping the
> https://gcc.gnu.org/pipermail/gcc-patches/2024-February/645644.html
> patch.
> After looking at this PR again yesterday, I'm convinced we don't need
> either this patch or some other patch to deal with the jump functions,
> but both, this patch to clear (or other variant would be to union them)
> SSA_NAME_RANGE_INFO in the bodies of IPA-ICF merged functions, and another
> patch that would maybe in sem_function::merge go through all the
> callees cgraph edges and for each of them attempt to merge (for value
> range union) at least the value range/pointer alignment information from
> the corresponding cgraph edge from alias for all jump functions of all
> the arguments.
> 
> Bootstrapped/regtested again last night.

I am sorry for delaying this.  I made the variant that simply compares
value range of functions and prevents merging if they diverge and wanted
to make some bigger statistics.  This made me notice some performance
problems on clang performance and libstdc++ RB-trees which disrailed me
from the original PR.  I will finish the statistics today.

For next stage1 we definitly want to move ahead with merging metadata
(not only value ranges, but hopefully also TBAA).  Currently the only
thing we merge aggressively is the edge profile (and we have PR on that
merging functions which differs only but likely/unlikely attribute).
However for backportability and for this avoiding merging may be safer
solution depending on the stats.A

I was looking into ipa-vrp useability and there are several tests on
Firefox talos where ipa-prop VRP makes difference. It boils down to
propagating fact that parameter is alway snon-NULL.  This is commonly
tested in C++ casts.

Honza

Re: Patch ping^2

[Jan Hubicka]

> Hi!
> 
> I'd like to ping 2 patches:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/644580.html
>   
>   
> PR113617 P1 - Handle private COMDAT function symbol reference in readonly 
> data section  
>   
>   
>   
>   
> More details in the   
>   
>   
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/thread.html#644121 
>   
>   
> and   
>   
>   
> https://gcc.gnu.org/pipermail/gcc-patches/2024-January/thread.html#644486 
>   
>   
> threads.  
>   
>   

I went through the thread - it looks like an issue that was latent for
quite some time.  I think it belongs to cgraph/symtab maintenance, so I
think I can OK this patch.

Honza
> 
> and
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2024-February/645326.html
> i386: Enable _BitInt support on ia32
> 
> all the FAILs mentioned in that mail have been fixed by now.
> 
> Thanks
> 
>   Jakub
> 

Re: Patch ping^2

[Uros Bizjak]

On Mon, Feb 26, 2024 at 10:33 AM Jakub Jelinek  wrote:
>
> Hi!
>
> I'd like to ping 2 patches:

> https://gcc.gnu.org/pipermail/gcc-patches/2024-February/645326.html
> i386: Enable _BitInt support on ia32
>
> all the FAILs mentioned in that mail have been fixed by now.

LGTM, based on HJ's advice.

Uros.

Re: Patch ping

[Jeff Law]

On 2/9/24 02:44, Jakub Jelinek wrote:



https://gcc.gnu.org/pipermail/gcc-patches/2024-February/644701.html
Introduce HOST_SIZE_T_PRINT_UNSIGNED etc. macros to fix LLP64 host build issue

Both have been successfully bootstrapped/regtested on x86_64-linux and
i686-linux, the latter has been tested by Jonathan on Windows too.
The alternative is start using %zu (AFAIK we only do that in libgccjit which
isn't supported everywhere and while it is C99, not sure if all supported
host C libraries support it), or change ira-conflicts.cc to
--- gcc/ira-conflicts.cc2024-02-01 21:03:57.339193085 +0100
+++ gcc/ira-conflicts.cc2024-02-09 10:41:39.201150644 +0100
@@ -151,8 +151,8 @@ build_conflict_bit_table (void)
  fprintf
(ira_dump_file,
 "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
-   (long) allocated_words_num * sizeof (IRA_INT_TYPE),
-   (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
+   (long) (allocated_words_num * sizeof (IRA_INT_TYPE)),
+   (long) (object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE)));
  
objects_live = sparseset_alloc (ira_objects_num);

for (i = 0; i < ira_max_point; i++)
Note, we have many more cases where we use %ld or %lu to print
size_t values (ideally %zd/%zu if we can assume it on all hosts, or
with the above introduced HOST_SIZE_T_PRINT*, the problem with the
%ld/%lu and casts is that it truncates the values on LLP64 hosts (aka
%Windows).
While I'd love to be able to use %z, I suspect it's going to be 
problematical.  That's one of the cleanups I need to have Mariam do on 
the CRC code which uses %z extensively in its debugging dumps.


So let's assume %z is a no-go for now.

Having a HOST_SIZE_T_PRINT_UNSIGNED seems useful, so I'd tend to prefer 
we go with that solution from the URL above rather than just throwing 
some parens around the expression before casting the result to "long".


jeff

Re: Patch ping: [PATCH] testsuite work-around compound-assignment-1.c C++ failures on various targets [PR111377]

[David Malcolm]

On Tue, 2023-09-19 at 09:20 +0200, Jakub Jelinek wrote:
> Hi!
> 
> On Tue, Sep 12, 2023 at 09:02:55AM +0200, Jakub Jelinek via Gcc-
> patches wrote:
> > On Mon, Sep 11, 2023 at 11:11:30PM +0200, Jakub Jelinek via Gcc-
> > patches wrote:
> > > On Mon, Sep 11, 2023 at 07:27:57PM +0200, Benjamin Priour via
> > > Gcc-patches wrote:
> > > > Thanks for the report,
> > > > 
> > > > After investigation it seems the location of the new dejagnu
> > > > directive for
> > > > C++ differs depending on the configuration.
> > > > The expected warning is still emitted, but its location differ
> > > > slightly.
> > > > I expect it to be not an issue per se of the analyzer, but a
> > > > divergence in
> > > > the FE between the two configurations.
> > > 
> > > I think the divergence is whether called_by_test_5b returns the
> > > struct
> > > in registers or in memory.  If in memory (like in the x86_64 -m32
> > > case), we have
> > >   [compound-assignment-1.c:71:21] D.3191 = called_by_test_5b ();
> > > [return slot optimization]
> > >   [compound-assignment-1.c:71:21 discrim 1] D.3191 ={v}
> > > {CLOBBER(eol)};
> > >   [compound-assignment-1.c:72:1] return;
> > > in the IL, while if in registers (like x86_64 -m64 case), just
> > >   [compound-assignment-1.c:71:21] D.3591 = called_by_test_5b ();
> > >   [compound-assignment-1.c:72:1] return;
> > > 
> > > If you just want to avoid the differences, putting } on the same
> > > line as the
> > > call might be a usable workaround for that.
> > 
> > Here is the workaround in patch form.  Tested on x86_64-linux -
> > m32/-m64, ok
> > for trunk?
> 
> I'd like to ping this patch.

OK

Dave

> 
> > 2023-09-12  Jakub Jelinek  
> > 
> > PR testsuite/111377
> > * c-c++-common/analyzer/compound-assignment-1.c (test_5b):
> > Move
> > closing } to the same line as the call to work-around
> > differences in
> > diagnostics line.
> > 
> > --- gcc/testsuite/c-c++-common/analyzer/compound-assignment-
> > 1.c.jj  2023-09-11 11:05:47.523727789 +0200
> > +++ gcc/testsuite/c-c++-common/analyzer/compound-assignment-
> > 1.c 2023-09-12 08:58:52.854231161 +0200
> > @@ -68,5 +68,8 @@ called_by_test_5b (void)
> >  
> >  void test_5b (void)
> >  {
> > -  called_by_test_5b ();
> > -} /* { dg-warning "leak of '.ptr_wrapper::ptr'" "" {
> > target c++ } } */
> > +  called_by_test_5b (); }
> > +/* { dg-warning "leak of '.ptr_wrapper::ptr'" "" {
> > target c++ } .-1 } */
> > +/* The closing } above is intentionally on the same line as the
> > call, because
> > +   otherwise the exact line of the diagnostics depends on whether
> > the
> > +   called_by_test_5b () call satisfies aggregate_value_p or not. 
> > */
> > 
> > 
> > Jakub
> 
> Jakub
> 

Re: Patch ping: Re: [PATCH] c, c++, v2: Accept __builtin_classify_type (typename)

[Joseph Myers]

On Mon, 18 Sep 2023, Jakub Jelinek via Gcc-patches wrote:

> Hi!
> 
> I'd like to ping this patch.
> The C++ FE part has been approved by Jason already with a minor change
> I've made in my copy.
> Are the remaining parts ok for trunk?

In the C front-end changes, since you end up discarding any side effects 
from the type, I'd expect use of in_alignof to be more appropriate than 
in_typeof (and thus not needing to use pop_maybe_used).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Patch ping Re: [PATCH 0/12] GCC _BitInt support [PR102989]

[Andrew Pinski via Gcc-patches]

On Mon, Aug 21, 2023 at 8:25 AM Jakub Jelinek via Gcc-patches
 wrote:
>
> Hi!
>
> On Wed, Aug 09, 2023 at 08:14:14PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > Jakub Jelinek (12):
> >   expr: Small optimization [PR102989]
> >   lto-streamer-in: Adjust assert [PR102989]
> >   phiopt: Fix phiopt ICE on vops [PR102989]
> >   Middle-end _BitInt support [PR102989]
> >   _BitInt lowering support [PR102989]
> >   i386: Enable _BitInt on x86-64 [PR102989]
> >   ubsan: _BitInt -fsanitize=undefined support [PR102989]
> >   libgcc: Generated tables for _BitInt <-> _Decimal* conversions [PR102989]
> >   libgcc _BitInt support [PR102989]
> >   C _BitInt support [PR102989]
> >   testsuite part 1 for _BitInt support [PR102989]
> >   testsuite part 2 for _BitInt support [PR102989]
>
> +   C _BitInt incremental fixes [PR102989]
>
> I'd like to ping this patch series.
> First 3 patches are committed, the rest awaits patch review.
>
> Joseph, could I ask now at least for an overall design review of the
> C patches (8-10,13) whether its interfaces with middle-end are ok,
> so that Richi can review the middle-end parts?

On a related note, does it make sense to add this as a C++ front-end
as an Extension too?
I noticed clang supports it for C++.

Thanks,
Andrew

>
> Thanks.
>
> Jakub
>

Re: Patch ping Re: [PATCH 0/12] GCC _BitInt support [PR102989]

[Joseph Myers]

On Mon, 21 Aug 2023, Jakub Jelinek via Gcc-patches wrote:

> Joseph, could I ask now at least for an overall design review of the
> C patches (8-10,13) whether its interfaces with middle-end are ok,
> so that Richi can review the middle-end parts?

I am fine with the interface to the middle-end parts.

I think the libgcc functions (i.e. those exported by libgcc, to which 
references are generated by the compiler) need documenting in libgcc.texi.  
Internal functions or macros in the libgcc patch need appropriate comments 
specifying their semantics; especially FP_TO_BITINT and FP_FROM_BITINT 
which have a lot of arguments and no comments saying what the semantics of 
the macros and their arguments are supposed to me.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Patch ping (Re: [PATCH] middle-end, i386: Pattern recognize add/subtract with carry [PR79173])

[Uros Bizjak via Gcc-patches]

On Tue, Jun 13, 2023 at 9:06 AM Jakub Jelinek  wrote:
>
> Hi!
>
> On Tue, Jun 06, 2023 at 11:42:07PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > The following patch introduces {add,sub}c5_optab and pattern recognizes
> > various forms of add with carry and subtract with carry/borrow, see
> > pr79173-{1,2,3,4,5,6}.c tests on what is matched.
> > Primarily forms with 2 __builtin_add_overflow or __builtin_sub_overflow
> > calls per limb (with just one for the least significant one), for
> > add with carry even when it is hand written in C (for subtraction
> > reassoc seems to change it too much so that the pattern recognition
> > doesn't work).  __builtin_{add,sub}_overflow are standardized in C23
> > under ckd_{add,sub} names, so it isn't any longer a GNU only extension.
> >
> > Note, clang has for these has (IMHO badly designed)
> > __builtin_{add,sub}c{b,s,,l,ll} builtins which don't add/subtract just
> > a single bit of carry, but basically add 3 unsigned values or
> > subtract 2 unsigned values from one, and result in carry out of 0, 1, or 2
> > because of that.  If we wanted to introduce those for clang compatibility,
> > we could and lower them early to just two __builtin_{add,sub}_overflow
> > calls and let the pattern matching in this patch recognize it later.
> >
> > I've added expanders for this on ix86 and in addition to that
> > added various peephole2s to make sure we get nice (and small) code
> > for the common cases.  I think there are other PRs which request that
> > e.g. for the _{addcarry,subborrow}_u{32,64} intrinsics, which the patch
> > also improves.
>
> I'd like to ping this patch.

I briefly went over the x86 part (LGTM), but please get a middle-end
approval first.

Thanks,
Uros.

>
> Thanks.
>
> Jakub
>

Re: Patch ping Re: [PATCH] ipa: Avoid another ICE when dealing with type-incompatibilities (PR 108959)

[Jan Hubicka via Gcc-patches]

> On Thu, Mar 23, 2023 at 11:09:19AM +0100, Martin Jambor wrote:
> > Hi,
> > 
> > PR 108959 shows one more example where undefined code with type
> > incompatible accesses to stuff passed in parameters can cause an ICE
> > because we try to create a VIEW_CONVERT_EXPR of mismatching sizes:
> > 
> > 1. IPA-CP tries to push one type from above,
> > 
> > 2. IPA-SRA (correctly) decides the parameter is useless because it is
> >only used to construct an argument to another function which does not
> >use it and so the formal parameter should be removed,
> > 
> > 3. but the code reconciling IPA-CP and IPA-SRA transformations still
> >wants to perform the IPA-CP and overrides the built-in DCE of
> >useless statements and tries to stuff constants into them
> >instead, constants of a type with mismatching type and size.
> > 
> > This patch avoids the situation in IPA-SRA by purging the IPA-CP
> > results from any "aggregate" constants that are passed in parameters
> > which are detected to be useless.  It also removes IPA value range and
> > bits information associated with removed parameters stored in the same
> > structure so that the useless information is not streamed later on.
> > 
> > Bootstrapped and LTO-bootstrapped and tested on x86_64-linux.  OK for
> > trunk?
> > 
> > gcc/ChangeLog:
> > 
> > 2023-03-22  Martin Jambor  
> > 
> > PR ipa/108959
> > * ipa-sra.cc (zap_useless_ipcp_results): New function.
> > (process_isra_node_results): Call it.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 2023-03-17  Martin Jambor  
> > 
> > PR ipa/108959
> > * gcc.dg/ipa/pr108959.c: New test.
OK,
thanks!
Honza

Re: Patch ping: Re: [PATCH] libgcc, i386, optabs, v2: Add __float{, un}tibf to libgcc and expand BF -> integral through SF intermediate [PR107703]

[Jeff Law via Gcc-patches]

On 3/1/23 05:32, Jakub Jelinek via Gcc-patches wrote:

Hi!

On Wed, Nov 16, 2022 at 12:51:14PM +0100, Jakub Jelinek via Gcc-patches wrote:

On Wed, Nov 16, 2022 at 10:06:17AM +0100, Jakub Jelinek via Gcc-patches wrote:

Thoughts on this?  I guess my preference would be the BF -> SF -> TI
path because we won't need to waste
 32: 00015e10   321 FUNCGLOBAL DEFAULT   13 
__fixbfti@@GCC_13.0.0
 89: 00015f60   299 FUNCGLOBAL DEFAULT   13 
__fixunsbfti@@GCC_13.0.0
If so, I'd need to cut the fix parts of the patch below and
do something in the middle-end.


Here is adjusted patch that does that.

2022-11-16  Jakub Jelinek  

PR target/107703
* optabs.cc (expand_fix): For conversions from BFmode to integral,
use shifts to convert it to SFmode first and then convert SFmode
to integral.

* soft-fp/floattibf.c: New file.
* soft-fp/floatuntibf.c: New file.
* config/i386/libgcc-glibc.ver: Export __float{,un}tibf @ GCC_13.0.0.
* config/i386/64/t-softfp (softfp_extras): Add floattibf and
floatuntibf.
(CFLAGS-floattibf.c, CFLAGS-floatunstibf.c): Add -msse2.


I'd like to ping the libgcc non-i386 part of this patch, Uros said the i386
part is ok but that one depends on the generic libgcc changes.
I'll ping the optabs.cc change separately.

https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606398.html
with more info in
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606382.html

The optab changes are OK too.
jeff

Re: Patch ping: Re: [PATCH] libgcc, i386, optabs, v2: Add __float{, un}tibf to libgcc and expand BF -> integral through SF intermediate [PR107703]

[Uros Bizjak via Gcc-patches]

On Fri, Mar 10, 2023 at 7:11 PM Ian Lance Taylor  wrote:
>
> Jakub Jelinek  writes:
>
> > On Wed, Mar 01, 2023 at 01:32:43PM +0100, Jakub Jelinek via Gcc-patches 
> > wrote:
> >> On Wed, Nov 16, 2022 at 12:51:14PM +0100, Jakub Jelinek via Gcc-patches 
> >> wrote:
> >> > On Wed, Nov 16, 2022 at 10:06:17AM +0100, Jakub Jelinek via
> >> > Gcc-patches wrote:
> >> > > Thoughts on this?  I guess my preference would be the BF -> SF -> TI
> >> > > path because we won't need to waste
> >> > > 32: 00015e10 321 FUNC GLOBAL DEFAULT 13
> >> > > __fixbfti@@GCC_13.0.0
> >> > > 89: 00015f60 299 FUNC GLOBAL DEFAULT 13
> >> > > __fixunsbfti@@GCC_13.0.0
> >> > > If so, I'd need to cut the fix parts of the patch below and
> >> > > do something in the middle-end.
> >> >
> >> > Here is adjusted patch that does that.
> >> >
> >> > 2022-11-16  Jakub Jelinek  
> >> >
> >> >PR target/107703
> >> >* optabs.cc (expand_fix): For conversions from BFmode to integral,
> >> >use shifts to convert it to SFmode first and then convert SFmode
> >> >to integral.
> >> >
> >> >* soft-fp/floattibf.c: New file.
> >> >* soft-fp/floatuntibf.c: New file.
> >> >* config/i386/libgcc-glibc.ver: Export __float{,un}tibf @ GCC_13.0.0.
> >> >* config/i386/64/t-softfp (softfp_extras): Add floattibf and
> >> >floatuntibf.
> >> >(CFLAGS-floattibf.c, CFLAGS-floatunstibf.c): Add -msse2.
> >>
> >> I'd like to ping the libgcc non-i386 part of this patch, Uros said the i386
> >> part is ok but that one depends on the generic libgcc changes.
> >> I'll ping the optabs.cc change separately.
> >>
> >> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606398.html
> >> with more info in
> >> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606382.html
> >
> > I'd like to ping this again.  I've posted the previously added
> > bfloat16 changes as well as the above 2 new files to libc-alpha as well
> > https://sourceware.org/pipermail/libc-alpha/2023-March/146246.html
> > if it makes the review easier.
>
>
> The libgcc parts of this are fine.  Thanks.

Also OK for the x86 part.

Thanks,
Uros.

Re: Patch ping: Re: [PATCH] libgcc, i386, optabs, v2: Add __float{, un}tibf to libgcc and expand BF -> integral through SF intermediate [PR107703]

[Ian Lance Taylor]

Jakub Jelinek  writes:

> On Wed, Mar 01, 2023 at 01:32:43PM +0100, Jakub Jelinek via Gcc-patches wrote:
>> On Wed, Nov 16, 2022 at 12:51:14PM +0100, Jakub Jelinek via Gcc-patches 
>> wrote:
>> > On Wed, Nov 16, 2022 at 10:06:17AM +0100, Jakub Jelinek via
>> > Gcc-patches wrote:
>> > > Thoughts on this?  I guess my preference would be the BF -> SF -> TI
>> > > path because we won't need to waste
>> > > 32: 00015e10 321 FUNC GLOBAL DEFAULT 13
>> > > __fixbfti@@GCC_13.0.0
>> > > 89: 00015f60 299 FUNC GLOBAL DEFAULT 13
>> > > __fixunsbfti@@GCC_13.0.0
>> > > If so, I'd need to cut the fix parts of the patch below and
>> > > do something in the middle-end.
>> > 
>> > Here is adjusted patch that does that.
>> > 
>> > 2022-11-16  Jakub Jelinek  
>> > 
>> >PR target/107703
>> >* optabs.cc (expand_fix): For conversions from BFmode to integral,
>> >use shifts to convert it to SFmode first and then convert SFmode
>> >to integral.
>> > 
>> >* soft-fp/floattibf.c: New file.
>> >* soft-fp/floatuntibf.c: New file.
>> >* config/i386/libgcc-glibc.ver: Export __float{,un}tibf @ GCC_13.0.0.
>> >* config/i386/64/t-softfp (softfp_extras): Add floattibf and
>> >floatuntibf.
>> >(CFLAGS-floattibf.c, CFLAGS-floatunstibf.c): Add -msse2.
>> 
>> I'd like to ping the libgcc non-i386 part of this patch, Uros said the i386
>> part is ok but that one depends on the generic libgcc changes.
>> I'll ping the optabs.cc change separately.
>> 
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606398.html
>> with more info in
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606382.html
>
> I'd like to ping this again.  I've posted the previously added
> bfloat16 changes as well as the above 2 new files to libc-alpha as well
> https://sourceware.org/pipermail/libc-alpha/2023-March/146246.html
> if it makes the review easier.


The libgcc parts of this are fine.  Thanks.

Ian

Re: Patch ping: [PATCH] cygwin: Don't try to support multilibs [PR107998]

[Jonathan Yong via Gcc-patches]

On 3/10/23 09:37, Jakub Jelinek wrote:

Hi!

I'd like to ping this patch (as I wrote a week ago, NightStrike has tested
it):



Thanks, pushed to master branch.

Re: Patch ping - [PATCH] file-prefix-map: Fix up -f*-prefix-map= [PR108464]

[Richard Purdie via Gcc-patches]

On Fri, 2023-03-10 at 09:05 +, Richard Biener wrote:
> On Fri, 10 Mar 2023, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > I'd like to ping these patches.  All 3 variants have been
> > bootstrapped/regtested on x86_64-linux and i686-linux, the last
> > one is my preference I guess.  The current state breaks e.g. ccache.
> > 
> > https://gcc.gnu.org/pipermail/gcc-patches/2023-January/610285.html
> >   - PR108464 - P1 - file-prefix-map: Fix up -f*-prefix-map= (3 variants)
> 
> Let's go with your preference - I was hoping on comments from Richard 
> Purdie as of his preference but then ..
> > 

Sorry, I hadn't realised you were waiting on me :(. 

I'd commented on the bug and thought that covered things. We should be
fine with the third option, sorry about the issue and thanks for
resolving it!

Cheers,

Richard

Re: Patch ping - [PATCH] tree: Use comdat tree_code_{type,length} even for C++11/14 [PR108634]

[Richard Biener via Gcc-patches]

On Fri, 10 Mar 2023, Jakub Jelinek wrote:

> Hi!
> 
> I'd like to ping this patch, which has been successfully
> bootstrapped/regtested on x86_64-linux and i686-linux:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611180.html
>   - PR108634 - P3 - tree: Use comdat tree_code_{type,length} even for C++11/14

OK.

Thanks,
Richard.

> Thanks
>   Jakub
> 
> On Thu, Feb 02, 2023 at 03:30:29PM +0100, Jakub Jelinek via Gcc-patches wrote:
> > The recent change to undo the tree_code_type/tree_code_length
> > excessive duplication apparently broke building the Linux kernel
> > plugin.  While it is certainly desirable that GCC plugins are built
> > with the same compiler as GCC has been built and with the same options
> > (at least the important ones), it might be hard to arrange that,
> > e.g. if gcc is built using a cross-compiler but the plugin then built
> > natively, or GCC isn't bootstrapped for other reasons, or just as in
> > the kernel case they were building the plugin with -std=gnu++11 while
> > the bootstrapped GCC has been built without any such option and so with
> > whatever the compiler defaulted to.
> > 
> > For C++17 and later tree_code_{type,length} are UNIQUE symbols with
> > those assembler names, while for C++11/14 they were
> > _ZL14tree_code_type and _ZL16tree_code_length.
> > 
> > The following patch uses a comdat var for those even for C++11/14
> > as suggested by Maciej Cencora.  Relying on weak attribute is not an
> > option because not all hosts support it and there are non-GNU system
> > compilers.  While we could use it unconditionally,
> > I think defining a template just to make it comdat is weird, and
> > the compiler itself is always built with the same compiler.
> > Plugins, being separate shared libraries, will have a separate copy of
> > the arrays if they are ODR-used in the plugin, so there is not a big
> > deal if e.g. cc1plus uses tree_code_type while plugin uses
> > _ZN19tree_code_type_tmplILi0EE14tree_code_typeE or vice versa.
> > 
> > Tested in non-bootstrapped build with both -std=gnu++17 and -std=gnu++11,
> > ok for trunk if it passes full bootstrap/regtest?
> > 
> > 2023-02-02  Jakub Jelinek  
> > 
> > PR plugins/108634
> > * tree-core.h (tree_code_type, tree_code_length): For C++11 or
> > C++14, don't declare as extern const arrays.
> > (tree_code_type_tmpl, tree_code_length_tmpl): New types with
> > static constexpr member arrays for C++11 or C++14.
> > * tree.h (TREE_CODE_CLASS): For C++11 or C++14 use
> > tree_code_type_tmpl <0>::tree_code_type instead of tree_code_type.
> > (TREE_CODE_LENGTH): For C++11 or C++14 use
> > tree_code_length_tmpl <0>::tree_code_length instead of
> > tree_code_length.
> > * tree.cc (tree_code_type, tree_code_length): Remove.
> > 
> > --- gcc/tree-core.h.jj  2023-01-27 10:51:27.575399052 +0100
> > +++ gcc/tree-core.h 2023-02-02 15:06:05.048665279 +0100
> > @@ -2285,19 +2285,27 @@ struct floatn_type_info {
> >  extern bool tree_contains_struct[MAX_TREE_CODES][64];
> >  
> >  /* Class of tree given its code.  */
> > -#if __cpp_inline_variables >= 201606L
> >  #define DEFTREECODE(SYM, NAME, TYPE, LENGTH) TYPE,
> >  #define END_OF_BASE_TREE_CODES tcc_exceptional,
> >  
> > +#if __cpp_inline_variables < 201606L
> > +template 
> > +struct tree_code_type_tmpl {
> > +  static constexpr enum tree_code_class tree_code_type[] = {
> > +#include "all-tree.def"
> > +  };
> > +};
> > +
> > +template 
> > +constexpr enum tree_code_class tree_code_type_tmpl::tree_code_type[];
> > +#else
> >  constexpr inline enum tree_code_class tree_code_type[] = {
> >  #include "all-tree.def"
> >  };
> > +#endif
> >  
> >  #undef DEFTREECODE
> >  #undef END_OF_BASE_TREE_CODES
> > -#else
> > -extern const enum tree_code_class tree_code_type[];
> > -#endif
> >  
> >  /* Each tree code class has an associated string representation.
> > These must correspond to the tree_code_class entries.  */
> > @@ -2305,18 +2313,27 @@ extern const char *const tree_code_class
> >  
> >  /* Number of argument-words in each kind of tree-node.  */
> >  
> > -#if __cpp_inline_variables >= 201606L
> >  #define DEFTREECODE(SYM, NAME, TYPE, LENGTH) LENGTH,
> >  #define END_OF_BASE_TREE_CODES 0,
> > +
> > +#if __cpp_inline_variables < 201606L
> > +template 
> > +struct tree_code_length_tmpl {
> > +  static constexpr unsigned char tree_code_length[] = {
> > +#include "all-tree.def"
> > +  };
> > +};
> > +
> > +template 
> > +constexpr unsigned char tree_code_length_tmpl::tree_code_length[];
> > +#else
> >  constexpr inline unsigned char tree_code_length[] = {
> >  #include "all-tree.def"
> >  };
> > +#endif
> >  
> >  #undef DEFTREECODE
> >  #undef END_OF_BASE_TREE_CODES
> > -#else
> > -extern const unsigned char tree_code_length[];
> > -#endif
> >  
> >  /* Vector of all alias pairs for global symbols.  */
> >  extern GTY(()) vec *alias_pairs;
> > --- gcc/tree.h.jj   2023-01-27 20:09:16.183970583 +0100
> > +++ gcc/tree.h  

Re: Patch ping - [PATCH] file-prefix-map: Fix up -f*-prefix-map= [PR108464]

[Richard Biener via Gcc-patches]

On Fri, 10 Mar 2023, Jakub Jelinek wrote:

> Hi!
> 
> I'd like to ping these patches.  All 3 variants have been
> bootstrapped/regtested on x86_64-linux and i686-linux, the last
> one is my preference I guess.  The current state breaks e.g. ccache.
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2023-January/610285.html
>   - PR108464 - P1 - file-prefix-map: Fix up -f*-prefix-map= (3 variants)

Let's go with your preference - I was hoping on comments from Richard 
Purdie as of his preference but then ..

Thanks,
Richard.

> Thanks
>   Jakub
> 
> On Fri, Jan 20, 2023 at 04:05:55PM +0100, Jakub Jelinek via Gcc-patches wrote:
> > On Tue, Nov 01, 2022 at 01:46:20PM -0600, Jeff Law via Gcc-patches wrote:
> > > > This does cause a change of behaviour if users were previously relying 
> > > > upon
> > > > symlinks or absolute paths not being resolved.
> > > 
> > > I'm not too worried about this scenario.
> > 
> > As mentioned in the PR, this patch breaks e.g. ccache testsuite.
> > 
> > I strongly doubt most of the users want such a behavior, because it
> > makes all filenames absolute when -f*-prefix-map= options remap one
> > absolute path to another one.
> > Say if I'm in /tmp and /tmp is the canonical path and there is
> > src/test.c file, with -fdebug-prefix-map=/tmp=/blah
> > previously there would be DW_AT_comp_dir "/blah" and it is still there,
> > but DW_AT_name which was previouly "src/test.c" (relative against
> > DW_AT_comp_dir) is now "/blah/src/test.c" instead.
> > 
> > Even worse, the canonicalization is only done on the remap_filename
> > argument, but not on the old_prefix side.  That is e.g. what breaks
> > ccache.  If there is
> > /tmp/foobar1 directory and
> > ln -sf foobar1 /tmp/foobar2
> > cd /tmp/foobar2
> > then -fdebug-prefix-map=`pwd`:/blah will just not work, while
> > src/test.c will be canonicalized to /tmp/foobar1/src/test.c,
> > old_prefix is still what the user provided which is /tmp/foobar2.
> > User would need to change their uses to use -fdebug-prefix-map=`realpath 
> > $(pwd)`=/blah
> > 
> > I'm attaching 3 so far just compile tested patches.
> > 
> > The first patch just reverts the patch (and its follow-up patch).
> > 
> > The second introduces a new option, -f{,no}-canon-prefix-map which affects
> > the behavior of -f{file,macro,debug,profile}-prefix-map=, if on it
> > canonicalizes the old path of the prefix map option and compares that
> > against the canonicalized filename for absolute paths but not relative.
> > 
> > And last is like the second, but does that also for relative paths except
> > for filenames with no / (or / or \ on DOS based fs).  So, the third patch
> > gets an optional behavior of what has been on the trunk lately with the
> > difference that the old_prefix is canonicalized by the compiler.
> > 
> > Initially I've thought I'd just add some magic syntax to the OLD=NEW
> > argument of those options (because there are 4 of them), but as noted
> > in the comments, = is valid char in OLD (just not new), so it would
> > be hard to figure out some syntax.  So instead a new option, which one
> > can turn on and off for different -f*-prefix-map= options if needed.
> > 
> > -fdebug-prefix-map=/path1=/mypath1 -fcanon-prefix-map \
> > -fdebug-prefix-map=/path2=/mypath2 -fno-canon-prefix-map \
> > -fdebug-prefix-map=/path3=/mypath3
> > 
> > will use the old behavior for the /path1 and /path3 handling and
> > the new one only for /path2 handling.
> > 
> > Thoughts on this?
> > 
> > Jakub
> 
> > 2023-01-20  Jakub Jelinek  
> > 
> > PR other/108464
> > * file-prefix-map.cc (remap_filename): Revert 2022-11-01 and 2022-11-07
> > changes.
> > 
> > --- gcc/file-prefix-map.cc
> > +++ gcc/file-prefix-map.cc
> > @@ -70,29 +70,19 @@ remap_filename (file_prefix_map *maps, const char 
> > *filename)
> >file_prefix_map *map;
> >char *s;
> >const char *name;
> > -  char *realname;
> >size_t name_len;
> >  
> > -  if (!filename || lbasename (filename) == filename)
> > -return filename;
> > -
> > -  realname = lrealpath (filename);
> > -
> >for (map = maps; map; map = map->next)
> > -if (filename_ncmp (realname, map->old_prefix, map->old_len) == 0)
> > +if (filename_ncmp (filename, map->old_prefix, map->old_len) == 0)
> >break;
> >if (!map)
> > -{
> > -  free (realname);
> > -  return filename;
> > -}
> > -  name = realname + map->old_len;
> > +return filename;
> > +  name = filename + map->old_len;
> >name_len = strlen (name) + 1;
> >  
> >s = (char *) ggc_alloc_atomic (name_len + map->new_len);
> >memcpy (s, map->new_prefix, map->new_len);
> >memcpy (s + map->new_len, name, name_len);
> > -  free (realname);
> >return s;
> >  }
> >  
> 
> > 2023-01-20  Jakub Jelinek  
> > 
> > PR other/108464
> > * common.opt (fcanon-prefix-map): New option.
> > * opts.cc: Include file-prefix-map.h.
> > (flag_canon_prefix_map): New variable.
> > (common_handle_option): Handle 

Re: Patch ping

[Joseph Myers]

On Wed, 1 Mar 2023, Jakub Jelinek via Gcc-patches wrote:

> Hi!
> 
> I'd like to ping a few pending patches:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607534.html
>   - PR107846 - P1 - c-family: Account for integral promotions of left shifts 
> for -Wshift-overflow warning

OK.

> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606973.html
>   - PR107465 - P2 - c-family: Fix up -Wsign-compare BIT_NOT_EXPR handling

OK.

> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607104.html
>   - PR107465 - P2 - c-family: Incremental fix for -Wsign-compare BIT_NOT_EXPR 
> handling

OK.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Patch ping

[Richard Sandiford via Gcc-patches]

Jakub Jelinek  writes:
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605965.html
>   - ABI - aarch64: Add bfloat16_t support for aarch64 (enabling it in GCC 14
> will be harder)

Sorry for the delay on this.  There's still an ongoing debate about
whether to keep the current AArch64 mangling or switch to the new one.

Thanks,
Richard

Re: [PATCH PING 2 (tree)] c++: source position of lambda captures [PR84471]

[Jason Merrill via Gcc-patches]

On 12/2/22 10:45, Jason Merrill wrote:

Tested x86_64-pc-linux-gnu, OK for trunk?

-- 8< --

If the DECL_VALUE_EXPR of a VAR_DECL has EXPR_LOCATION set, then any use of
that variable looks like it has that location, which leads to the debugger
jumping back and forth for both lambdas and structured bindings.

Rather than fix all the uses, it seems simplest to remove any EXPR_LOCATION
when setting DECL_VALUE_EXPR.  So the cp/ hunks aren't necessary, but it
seems cleaner not to work to add a location that will immediately get
stripped.

PR c++/84471
PR c++/107504

gcc/cp/ChangeLog:

* coroutines.cc (transform_local_var_uses): Don't
specify a location for DECL_VALUE_EXPR.
* decl.cc (cp_finish_decomp): Likewise.

gcc/ChangeLog:

* tree.cc (decl_value_expr_insert): Clear EXPR_LOCATION.

gcc/testsuite/ChangeLog:

* g++.dg/tree-ssa/value-expr1.C: New test.
* g++.dg/tree-ssa/value-expr2.C: New test.
* g++.dg/analyzer/pr93212.C: Move warning.
---
  gcc/cp/coroutines.cc|  4 ++--
  gcc/cp/decl.cc  | 12 +++---
  gcc/testsuite/g++.dg/analyzer/pr93212.C |  4 ++--
  gcc/testsuite/g++.dg/tree-ssa/value-expr1.C | 16 +
  gcc/testsuite/g++.dg/tree-ssa/value-expr2.C | 26 +
  gcc/tree.cc |  3 +++
  6 files changed, 52 insertions(+), 13 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/value-expr1.C
  create mode 100644 gcc/testsuite/g++.dg/tree-ssa/value-expr2.C

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 01a3e831ee5..a72bd6bbef0 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2047,8 +2047,8 @@ transform_local_var_uses (tree *stmt, int *do_subtree, 
void *d)
= lookup_member (lvd->coro_frame_type, local_var.field_id,
 /*protect=*/1, /*want_type=*/0,
 tf_warning_or_error);
- tree fld_idx = build3_loc (lvd->loc, COMPONENT_REF, TREE_TYPE (lvar),
-lvd->actor_frame, fld_ref, NULL_TREE);
+ tree fld_idx = build3 (COMPONENT_REF, TREE_TYPE (lvar),
+lvd->actor_frame, fld_ref, NULL_TREE);
  local_var.field_idx = fld_idx;
  SET_DECL_VALUE_EXPR (lvar, fld_idx);
  DECL_HAS_VALUE_EXPR_P (lvar) = true;
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 7af0b05d5f8..59e21581503 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -9133,9 +9133,7 @@ cp_finish_decomp (tree decl, tree first, unsigned int 
count)
  if (processing_template_decl)
continue;
  tree t = unshare_expr (dexp);
- t = build4_loc (DECL_SOURCE_LOCATION (v[i]), ARRAY_REF,
- eltype, t, size_int (i), NULL_TREE,
- NULL_TREE);
+ t = build4 (ARRAY_REF, eltype, t, size_int (i), NULL_TREE, NULL_TREE);
  SET_DECL_VALUE_EXPR (v[i], t);
  DECL_HAS_VALUE_EXPR_P (v[i]) = 1;
}
@@ -9154,9 +9152,7 @@ cp_finish_decomp (tree decl, tree first, unsigned int 
count)
  if (processing_template_decl)
continue;
  tree t = unshare_expr (dexp);
- t = build1_loc (DECL_SOURCE_LOCATION (v[i]),
- i ? IMAGPART_EXPR : REALPART_EXPR, eltype,
- t);
+ t = build1 (i ? IMAGPART_EXPR : REALPART_EXPR, eltype, t);
  SET_DECL_VALUE_EXPR (v[i], t);
  DECL_HAS_VALUE_EXPR_P (v[i]) = 1;
}
@@ -9180,9 +9176,7 @@ cp_finish_decomp (tree decl, tree first, unsigned int 
count)
  tree t = unshare_expr (dexp);
  convert_vector_to_array_for_subscript (DECL_SOURCE_LOCATION (v[i]),
 , size_int (i));
- t = build4_loc (DECL_SOURCE_LOCATION (v[i]), ARRAY_REF,
- eltype, t, size_int (i), NULL_TREE,
- NULL_TREE);
+ t = build4 (ARRAY_REF, eltype, t, size_int (i), NULL_TREE, NULL_TREE);
  SET_DECL_VALUE_EXPR (v[i], t);
  DECL_HAS_VALUE_EXPR_P (v[i]) = 1;
}
diff --git a/gcc/testsuite/g++.dg/analyzer/pr93212.C 
b/gcc/testsuite/g++.dg/analyzer/pr93212.C
index 41507e2b837..1029e8d547b 100644
--- a/gcc/testsuite/g++.dg/analyzer/pr93212.C
+++ b/gcc/testsuite/g++.dg/analyzer/pr93212.C
@@ -4,8 +4,8 @@
  auto lol()
  {
  int aha = 3;
-return [] { // { dg-warning "dereferencing pointer '.*' to within stale 
stack frame" }
-return aha;
+return [] {
+return aha; // { dg-warning "dereferencing pointer '.*' to within stale 
stack frame" }
  };
  /* TODO: may be worth special-casing the reporting of dangling
 references from lambdas, to highlight the declaration, and maybe fix
diff --git a/gcc/testsuite/g++.dg/tree-ssa/value-expr1.C 
b/gcc/testsuite/g++.dg/tree-ssa/value-expr1.C
new file mode 100644
index 

Re: [PATCH PING] build: add -Wconditionally-supported to strict_warn [PR64867]

[Jason Merrill via Gcc-patches]

On 12/6/22 08:26, Jason Merrill wrote:

Tested x86_64-pc-linux-gnu, OK for trunk?


Ping.


-- 8< --

The PR (which isn't resolved by this commit) pointed out to me that GCC
should build with -Wconditionally-supported to support bootstrapping with a
C++11 compiler that makes different choices.

PR c++/64867

gcc/ChangeLog:

* configure.ac (strict_warn): Add -Wconditionally-supported.
* configure: Regenerate.
---
  gcc/configure.ac | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/configure.ac b/gcc/configure.ac
index 7ca08726efa..12771fc292c 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -580,7 +580,7 @@ ACX_PROG_CC_WARNING_OPTS(
m4_quote(m4_do([-Wstrict-prototypes -Wmissing-prototypes ],
   [])), [c_loose_warn])
  ACX_PROG_CXX_WARNING_OPTS(
-   m4_quote(m4_do([-Wmissing-format-attribute ],
+   m4_quote(m4_do([-Wmissing-format-attribute ], 
[-Wconditionally-supported ],
   [-Woverloaded-virtual])), [strict_warn])
  ACX_PROG_CC_WARNING_OPTS(
m4_quote(m4_do([-Wold-style-definition -Wc++-compat])), [c_strict_warn])

base-commit: d19aa6af6634b1e97f38431ad091f3b3f12baf2f

Re: [PATCH][PING][sanitizer/106558] asan: fix unsafe optimization of Asan checks.

[Martin Liška]

On 11/25/22 17:18, Martin Liška wrote:

On 11/21/22 11:02, Jakub Jelinek wrote:

Otherwise LGTM.  Thanks and sorry for the review delay.


Yuri, do you want to commit the patch soon?

If not, I can help if you want?


Hey.

I've just installed the patch with function signature change
and changelog tweak. I'm testing multiple ASAN bugs and I need
this patch as it fixes quite something.

Thanks,
Martin



Cheers,
Martin

Re: [PATCH][PING][sanitizer/106558] asan: fix unsafe optimization of Asan checks.

[Martin Liška]

On 11/21/22 11:02, Jakub Jelinek wrote:
> Otherwise LGTM.  Thanks and sorry for the review delay.

Yuri, do you want to commit the patch soon?

If not, I can help if you want?

Cheers,
Martin

Re: [PATCH][PING][sanitizer/106558] asan: fix unsafe optimization of Asan checks.

[Jakub Jelinek via Gcc-patches]

On Mon, Nov 21, 2022 at 12:57:15PM +0300, Yuri Gribov wrote:
> From 4729f2db3f1b6b40ef0124e4a645788d7f66f426 Mon Sep 17 00:00:00 2001
> From: Yuri Gribov 
> Date: Sun, 14 Aug 2022 08:42:44 +0300
> Subject: [PATCH] asan: fix unsafe optimization of Asan checks.
> 
> gcc/
> PR sanitizer/106558
> * sanopt.c: Do not optimize out checks for non-SSA addresses.
> 
> gcc/testsuite/
> PR sanitizer/106558
> * c-c++-common/asan/pr106558.c: New test.
> ---
>  gcc/sanopt.cc  | 40 +-
>  gcc/testsuite/c-c++-common/asan/pr106558.c | 23 +
>  2 files changed, 54 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/asan/pr106558.c
> 
> diff --git a/gcc/sanopt.cc b/gcc/sanopt.cc
> index e9d188d7889..13942a0b1da 100644
> --- a/gcc/sanopt.cc
> +++ b/gcc/sanopt.cc
> @@ -80,16 +80,16 @@ struct sanopt_info
>  
>  /* If T has a single definition of form T = T2, return T2.  */
>  
> -static tree
> +static gimple *
>  maybe_get_single_definition (tree t)
>  {
>if (TREE_CODE (t) == SSA_NAME)
>  {
>gimple *g = SSA_NAME_DEF_STMT (t);
>if (gimple_assign_single_p (g))
> - return gimple_assign_rhs1 (g);
> + return g;
>  }
> -  return NULL_TREE;
> +  return NULL;
>  }
>  
>  /* Tree triplet for vptr_check_map.  */
> @@ -618,11 +618,30 @@ maybe_optimize_ubsan_vptr_ifn (class sanopt_ctx *ctx, 
> gimple *stmt)
>return true;
>  }
>  
> +/* Checks whether value of T in CHECK and USE is the same.  */
> +
> +static bool same_value_p (gimple *check, gimple *use, tree t)

Formatting.  Function name should be on another line:
static bool
same_value_p (gimple *check, gimple *use, tree t)

Otherwise LGTM.  Thanks and sorry for the review delay.

Jakub

Re: Patch ping (Re: [PATCH] libgcc: Decrease size of _Unwind_FrameState and even more size of cleared area in uw_frame_state_for)

[Joseph Myers]

I'm seeing the following build failure for arc-linux-gnu after this 
commit.  (Note that this is for building GCC *after* glibc, not for an 
initial inhibit_libc bootstrap build of GCC.)

In file included from 
/scratch/jmyers/glibc-bot/src/gcc/libgcc/unwind-dw2.c:413:
./md-unwind-support.h: In function 'arc_fallback_frame_state':
./md-unwind-support.h:103:10: warning: assignment to 'struct sigcontext *' from 
incompatible pointer type 'mcontext_t *' [-Wincompatible-pointer-types]
  103 |   sc = _->uc.uc_mcontext;
  |  ^
./md-unwind-support.h:118:45: error: 'struct ' has no member named 
'how'
  118 |   fs->regs.reg[register_id_for_index[i]].how = REG_SAVED_OFFSET;
  | ^
/scratch/jmyers/glibc-bot/src/gcc/libgcc/shared-object.mk:14: recipe for target 
'unwind-dw2.o' failed

https://sourceware.org/pipermail/libc-testresults/2022q4/010333.html

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Patch ping (Re: [PATCH] libgcc: Decrease size of _Unwind_FrameState and even more size of cleared area in uw_frame_state_for)

[Richard Biener via Gcc-patches]

On Wed, Oct 5, 2022 at 12:34 PM Jakub Jelinek via Gcc-patches
 wrote:
>
> Hi!
>
> I'd like to ping this patch.

The patch is OK.

Richard.

> Thanks.
>
> > 2022-09-19  Jakub Jelinek  
> >
> >   * unwind-dw2.h (REG_UNSAVED, REG_SAVED_OFFSET, REG_SAVED_REG,
> >   REG_SAVED_EXP, REG_SAVED_VAL_OFFSET, REG_SAVED_VAL_EXP,
> >   REG_UNDEFINED): New anonymous enum, moved from inside of
> >   struct frame_state_reg_info.
> >   (struct frame_state_reg_info): Remove reg[].how element and the
> >   anonymous enum there.  Add how element.
> >   * unwind-dw2.c: Include stddef.h.
> >   (uw_frame_state_for): Don't clear first
> >   offsetof (_Unwind_FrameState, regs.how[0]) bytes of *fs.
> >   (execute_cfa_program, __frame_state_for, uw_update_context_1,
> >   uw_update_context): Use fs->regs.how[X] instead of fs->regs.reg[X].how
> >   or fs.regs.how[X] instead of fs.regs.reg[X].how.
> >   * config/sh/linux-unwind.h (sh_fallback_frame_state): Likewise.
> >   * config/bfin/linux-unwind.h (bfin_fallback_frame_state): Likewise.
> >   * config/pa/linux-unwind.h (pa32_fallback_frame_state): Likewise.
> >   * config/pa/hpux-unwind.h (UPDATE_FS_FOR_SAR, UPDATE_FS_FOR_GR,
> >   UPDATE_FS_FOR_FR, UPDATE_FS_FOR_PC, pa_fallback_frame_state):
> >   Likewise.
> >   * config/alpha/vms-unwind.h (alpha_vms_fallback_frame_state):
> >   Likewise.
> >   * config/alpha/linux-unwind.h (alpha_fallback_frame_state): Likewise.
> >   * config/arc/linux-unwind.h (arc_fallback_frame_state,
> >   arc_frob_update_context): Likewise.
> >   * config/riscv/linux-unwind.h (riscv_fallback_frame_state): Likewise.
> >   * config/nios2/linux-unwind.h (NIOS2_REG): Likewise.
> >   * config/nds32/linux-unwind.h (NDS32_PUT_FS_REG): Likewise.
> >   * config/s390/tpf-unwind.h (s390_fallback_frame_state): Likewise.
> >   * config/s390/linux-unwind.h (s390_fallback_frame_state): Likewise.
> >   * config/sparc/sol2-unwind.h (sparc64_frob_update_context,
> >   MD_FALLBACK_FRAME_STATE_FOR): Likewise.
> >   * config/sparc/linux-unwind.h (sparc64_fallback_frame_state,
> >   sparc64_frob_update_context, sparc_fallback_frame_state): Likewise.
> >   * config/i386/sol2-unwind.h (x86_64_fallback_frame_state,
> >   x86_fallback_frame_state): Likewise.
> >   * config/i386/w32-unwind.h (i386_w32_fallback_frame_state): Likewise.
> >   * config/i386/linux-unwind.h (x86_64_fallback_frame_state,
> >   x86_fallback_frame_state): Likewise.
> >   * config/i386/freebsd-unwind.h (x86_64_freebsd_fallback_frame_state):
> >   Likewise.
> >   * config/i386/dragonfly-unwind.h
> >   (x86_64_dragonfly_fallback_frame_state): Likewise.
> >   * config/i386/gnu-unwind.h (x86_gnu_fallback_frame_state): Likewise.
> >   * config/csky/linux-unwind.h (csky_fallback_frame_state): Likewise.
> >   * config/aarch64/linux-unwind.h (aarch64_fallback_frame_state):
> >   Likewise.
> >   * config/aarch64/freebsd-unwind.h
> >   (aarch64_freebsd_fallback_frame_state): Likewise.
> >   * config/aarch64/aarch64-unwind.h (aarch64_frob_update_context):
> >   Likewise.
> >   * config/or1k/linux-unwind.h (or1k_fallback_frame_state): Likewise.
> >   * config/mips/linux-unwind.h (mips_fallback_frame_state): Likewise.
> >   * config/loongarch/linux-unwind.h (loongarch_fallback_frame_state):
> >   Likewise.
> >   * config/m68k/linux-unwind.h (m68k_fallback_frame_state): Likewise.
> >   * config/xtensa/linux-unwind.h (xtensa_fallback_frame_state):
> >   Likewise.
> >   * config/rs6000/darwin-fallback.c (set_offset): Likewise.
> >   * config/rs6000/aix-unwind.h (MD_FROB_UPDATE_CONTEXT): Likewise.
> >   * config/rs6000/linux-unwind.h (ppc_fallback_frame_state): Likewise.
> >   * config/rs6000/freebsd-unwind.h (frob_update_context): Likewise.
>
> Jakub
>

Re: Patch ping^3 ([PATCH] libstdc++: Outline the overlapping case of string _M_replace into a separate function [PR105329])

[Jonathan Wakely via Gcc-patches]

On Mon, 12 Sept 2022 at 10:16, Jakub Jelinek  wrote:
>
> On Wed, Aug 31, 2022 at 11:38:58AM +0200, Jakub Jelinek via Gcc-patches wrote:
> > On Wed, Aug 10, 2022 at 01:27:51PM +0200, Jakub Jelinek via Gcc-patches 
> > wrote:
> > > On Wed, Jul 27, 2022 at 11:33:29AM +0200, Jakub Jelinek via Gcc-patches 
> > > wrote:
> > > > The following patch is partially a workaround for bogus warnings
> > > > when the compiler isn't able to fold _M_disjunct call into constant
> > > > false, but also an optimization attempt - assuming _M_disjunct (__s)
> > > > is rare, the patch should shrink code size for the common case and
> > > > use library or for non-standard instantiations an out of line
> > > > function to handle the rare case.
> > >
> > > I'd like to ping this patch.
> >
> > I'd like to ping this patch again.
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598896.html
>
> Ping again.


OK for trunk, sorry for the slow review.

Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)

[Jonathan Wakely via Gcc-patches]

On Fri, 9 Sept 2022 at 20:01, Thomas Rodgers wrote:
>
> s/__weak/__is_weak/g  perhaps?

Yes, that'll do. Fixed by the attached, with a test to avoid it happening again.

Tested x86_64-linux, pushed to trunk.




>
> On Fri, Sep 9, 2022 at 11:46 AM Iain Sandoe via Libstdc++ 
>  wrote:
>>
>>
>>
>> > On 9 Sep 2022, at 19:36, Rainer Orth  wrote:
>> >
>>
>> >> Here's a complete patch that combines the various incremental patches
>> >> that have been going around. I'm testing this now.
>> >>
>> >> Please take a look.
>> >
>> > unfortunately, this patch broke macOS bootstrap (seen on
>> > x86_64-apple-darwin11.4.2):
>> >
>> > In file included from 
>> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33,
>> > from 
>> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78,
>> > from 
>> > /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82:
>> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:
>> >  In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, 
>> > _Val<_Tp>&, _Val<_Tp>&, bool, std::memory_order, std::memory_order)':
>> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49:
>> >  error: expected primary-expression before ',' token
>> > 1008 |   __weak, int(__s), 
>> > int(__f)))
>> >  | ^
>> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50:
>> >  error: expected primary-expression before ',' token
>> > 1017 |__weak, int(__s), 
>> > int(__f));
>> >  |  ^
>> >
>> > Darwin gcc predefines __weak= in gcc/config/darwin-c.cc 
>> > (darwin_cpp_builtins).
>>
>> yes, __weak and __strong are Objective C things (in principle, applicable to 
>> non-Darwin targets
>> using NeXT runtime - there is at least one such target).
>>
>> Iain
>>
commit 007680f946eaffa3c6321624129e1ec18e673091
Author: Jonathan Wakely 
Date:   Fri Sep 9 21:03:58 2022

libstdc++: Rename parameter to avoid darwin __weak qualifier

libstdc++-v3/ChangeLog:

* include/bits/atomic_base.h (__atomic_impl::__compare_exchange):
Rename __weak to __is_weak.
* testsuite/17_intro/names.cc: Add __weak and __strong.

diff --git a/libstdc++-v3/include/bits/atomic_base.h 
b/libstdc++-v3/include/bits/atomic_base.h
index 29315547aab..6ea3268fdf0 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -990,7 +990,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 template
   _GLIBCXX_ALWAYS_INLINE bool
   __compare_exchange(_Tp& __val, _Val<_Tp>& __e, _Val<_Tp>& __i,
-bool __weak, memory_order __s, memory_order __f) 
noexcept
+bool __is_weak,
+memory_order __s, memory_order __f) noexcept
   {
__glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
 
@@ -1005,7 +1006,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__atomic_impl::__clear_padding(*__exp);
if (__atomic_compare_exchange(std::__addressof(__val), __exp,
  __atomic_impl::__clear_padding(__i),
- __weak, int(__s), int(__f)))
+ __is_weak, int(__s), int(__f)))
  return true;
__builtin_memcpy(std::__addressof(__e), __exp, sizeof(_Vp));
return false;
@@ -1014,7 +1015,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  return __atomic_compare_exchange(std::__addressof(__val),
   std::__addressof(__e),
   std::__addressof(__i),
-  __weak, int(__s), int(__f));
+  __is_weak, int(__s), int(__f));
   }
   } // namespace __atomic_impl
 
diff --git a/libstdc++-v3/testsuite/17_intro/names.cc 
b/libstdc++-v3/testsuite/17_intro/names.cc
index ede2fe8caa7..86fb8f8999b 100644
--- a/libstdc++-v3/testsuite/17_intro/names.cc
+++ b/libstdc++-v3/testsuite/17_intro/names.cc
@@ -129,6 +129,10 @@
 // This clashes with newlib so don't use it.
 # define __lockablecannot be used as an identifier
 
+#ifndef __APPLE__
+#define __weak   predefined qualifier on darwin
+#define __strong predefined qualifier on darwin
+#endif
 
 // Common template parameter names
 #define OutputIterator OutputIterator is not a reserved name

Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)

[Thomas Rodgers via Gcc-patches]

s/__weak/__is_weak/g  perhaps?

On Fri, Sep 9, 2022 at 11:46 AM Iain Sandoe via Libstdc++ <
libstd...@gcc.gnu.org> wrote:

>
>
> > On 9 Sep 2022, at 19:36, Rainer Orth 
> wrote:
> >
>
> >> Here's a complete patch that combines the various incremental patches
> >> that have been going around. I'm testing this now.
> >>
> >> Please take a look.
> >
> > unfortunately, this patch broke macOS bootstrap (seen on
> > x86_64-apple-darwin11.4.2):
> >
> > In file included from
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33,
> > from
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78,
> > from
> /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82:
> >
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:
> In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&,
> _Val<_Tp>&, bool, std::memory_order, std::memory_order)':
> >
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49:
> error: expected primary-expression before ',' token
> > 1008 |   __weak, int(__s),
> int(__f)))
> >  | ^
> >
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50:
> error: expected primary-expression before ',' token
> > 1017 |__weak, int(__s),
> int(__f));
> >  |  ^
> >
> > Darwin gcc predefines __weak= in gcc/config/darwin-c.cc
> (darwin_cpp_builtins).
>
> yes, __weak and __strong are Objective C things (in principle, applicable
> to non-Darwin targets
> using NeXT runtime - there is at least one such target).
>
> Iain
>
>

Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)

[Iain Sandoe via Gcc-patches]

> On 9 Sep 2022, at 19:36, Rainer Orth  wrote:
> 

>> Here's a complete patch that combines the various incremental patches
>> that have been going around. I'm testing this now.
>> 
>> Please take a look.
> 
> unfortunately, this patch broke macOS bootstrap (seen on
> x86_64-apple-darwin11.4.2):
> 
> In file included from 
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33,
> from 
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78,
> from 
> /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82:
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:
>  In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&, 
> _Val<_Tp>&, bool, std::memory_order, std::memory_order)':
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49:
>  error: expected primary-expression before ',' token
> 1008 |   __weak, int(__s), int(__f)))
>  | ^
> /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50:
>  error: expected primary-expression before ',' token
> 1017 |__weak, int(__s), int(__f));
>  |  ^
> 
> Darwin gcc predefines __weak= in gcc/config/darwin-c.cc (darwin_cpp_builtins).

yes, __weak and __strong are Objective C things (in principle, applicable to 
non-Darwin targets
using NeXT runtime - there is at least one such target).

Iain

Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)

[Rainer Orth]

Hi Jonathan,

> Here's a complete patch that combines the various incremental patches
> that have been going around. I'm testing this now.
>
> Please take a look.

unfortunately, this patch broke macOS bootstrap (seen on
x86_64-apple-darwin11.4.2):

In file included from 
/var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33,
 from 
/var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78,
 from 
/vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82:
/var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:
 In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&, 
_Val<_Tp>&, bool, std::memory_order, std::memory_order)':
/var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49:
 error: expected primary-expression before ',' token
 1008 |   __weak, int(__s), int(__f)))
  | ^
/var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50:
 error: expected primary-expression before ',' token
 1017 |__weak, int(__s), int(__f));
  |  ^

Darwin gcc predefines __weak= in gcc/config/darwin-c.cc (darwin_cpp_builtins).

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)

[Thomas Rodgers via Gcc-patches]

Looks good to me.

Tom.

On Wed, Sep 7, 2022 at 4:56 AM Jonathan Wakely  wrote:

> Here's a complete patch that combines the various incremental patches
> that have been going around. I'm testing this now.
>
> Please take a look.
>

Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)

[Jonathan Wakely via Gcc-patches]

Here's a complete patch that combines the various incremental patches
that have been going around. I'm testing this now.

Please take a look.
commit 4a0a8ec5bc2a890a1568f99eace666e9f72d
Author: Thomas Rodgers 
Date:   Thu Aug 25 11:11:40 2022

libstdc++: Clear padding bits in atomic compare_exchange

This change implements P0528 which requires that padding bits not
participate in atomic compare exchange operations. All arguments to the
generic template are 'sanitized' by the __builtin_clear_padding intrinsic
before they are used in comparisons. This requires that any stores
also sanitize the incoming value.

Co-authored-by: Jakub Jelinek 
Co-authored-by: Jonathan Wakely 

Signed-off-by: Thomas Rodgers 

libstdc++-v3/ChangeLog:

* include/bits/atomic_base.h (__atomic_impl::__maybe_has_padding):
New function.
(__atomic_impl::clear_padding): Likewise.
(__atomic_impl::__compare_exchange): Likewise.
(__atomic_impl::compare_exchange_weak): Delegate to
__compare_exchange.
(__atomic_impl::compare_exchange_strong): Likewise.
* include/std/atomic (atomic::atomic(T)): Clear padding when
possible in a constexpr function.
(atomic::store): Clear padding.
(atomic::exchange): Likewise.
(atomic::compare_exchange_weak): Use __compare_exchange.
(atomic::compare_exchange_strong): Likewise.
* testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
test.
* testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc:
New test.

diff --git a/libstdc++-v3/include/bits/atomic_base.h 
b/libstdc++-v3/include/bits/atomic_base.h
index d29e4434177..29315547aab 100644
--- a/libstdc++-v3/include/bits/atomic_base.h
+++ b/libstdc++-v3/include/bits/atomic_base.h
@@ -33,6 +33,7 @@
 #pragma GCC system_header
 
 #include 
+#include  // For placement new
 #include 
 #include 
 #include 
@@ -952,19 +953,76 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   { return __atomic_fetch_sub(&_M_p, _M_type_size(__d), int(__m)); }
 };
 
-  /// @endcond
+  namespace __atomic_impl
+  {
+// Implementation details of atomic padding handling
+
+template
+  constexpr bool
+  __maybe_has_padding()
+  {
+#if ! __has_builtin(__builtin_clear_padding)
+   return false;
+#elif __has_builtin(__has_unique_object_representations)
+   return !__has_unique_object_representations(_Tp)
+ && !is_same<_Tp, float>::value && !is_same<_Tp, double>::value;
+#else
+   return true;
+#endif
+  }
+
+template
+  _GLIBCXX_ALWAYS_INLINE _Tp*
+  __clear_padding(_Tp& __val) noexcept
+  {
+   auto* __ptr = std::__addressof(__val);
+#if __has_builtin(__builtin_clear_padding)
+   if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>())
+ __builtin_clear_padding(__ptr);
+#endif
+   return __ptr;
+  }
+
+// Remove volatile and create a non-deduced context for value arguments.
+template
+  using _Val = typename remove_volatile<_Tp>::type;
+
+template
+  _GLIBCXX_ALWAYS_INLINE bool
+  __compare_exchange(_Tp& __val, _Val<_Tp>& __e, _Val<_Tp>& __i,
+bool __weak, memory_order __s, memory_order __f) 
noexcept
+  {
+   __glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
+
+   using _Vp = _Val<_Tp>;
+
+   if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Vp>())
+ {
+   // We must not modify __e on success, so cannot clear its padding.
+   // Copy into a buffer and clear that, then copy back on failure.
+   alignas(_Vp) unsigned char __buf[sizeof(_Vp)];
+   _Vp* __exp = ::new((void*)__buf) _Vp(__e);
+   __atomic_impl::__clear_padding(*__exp);
+   if (__atomic_compare_exchange(std::__addressof(__val), __exp,
+ __atomic_impl::__clear_padding(__i),
+ __weak, int(__s), int(__f)))
+ return true;
+   __builtin_memcpy(std::__addressof(__e), __exp, sizeof(_Vp));
+   return false;
+ }
+   else
+ return __atomic_compare_exchange(std::__addressof(__val),
+  std::__addressof(__e),
+  std::__addressof(__i),
+  __weak, int(__s), int(__f));
+  }
+  } // namespace __atomic_impl
 
 #if __cplusplus > 201703L
-  /// @cond undocumented
-
   // Implementation details of atomic_ref and atomic.
   namespace __atomic_impl
   {
-// Remove volatile and create a non-deduced context for value arguments.
-template
-  using _Val = remove_volatile_t<_Tp>;
-
-// As above, but for difference_type arguments.
+// Like _Val above, but for difference_type arguments.
 template
 

Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)

[Thomas Rodgers via Gcc-patches]

Sorry for the delay in getting to this.

I am currently working on moving the bulk of the atomic wait implementation
into the .so. I'd like to get that work to a stable state before revisiting
this patch, but obviously if we want this to make it into GCC13, it needs
to happen sooner rather than later.

On Thu, Aug 25, 2022 at 3:11 AM Jakub Jelinek  wrote:

> On Tue, Jan 18, 2022 at 09:48:19PM +, Jonathan Wakely via Gcc-patches
> wrote:
> > On Tue, 2 Nov 2021 at 01:26, Thomas Rodgers  wrote:
> >
> > > This should address Jonathan's feedback and adds support for
> atomic_ref
> > >
> >
> >
> > >This change implements P0528 which requires that padding bits not
> > >participate in atomic compare exchange operations. All arguments to the
> > >generic template are 'sanitized' by the __builtin_clearpadding intrisic
> >
> > The name of the intrinsic and the word "instrinsic" have typos.
>
> I'd like to ping this patch.
> To make some progress, I've tried to incorporate some of Jonathan's
> review comments below, but it is incomplete.
>
> ChangeLog + wording above it fixed.
>
> > >
> > >   explicit
> > >   __atomic_ref(_Tp& __t) : _M_ptr(std::__addressof(__t))
> > >-  { __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) ==
> 0); }
> > >+  {
> > >+ __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0);
> > >+#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding)
> > >+ __builtin_clear_padding(_M_ptr);
> > >+#endif
> > >+  }
> >
> > Is this safe to do?
> >
> > What if multiple threads all create a std::atomic_ref round the same
> object
> > at once, they'll all try to clear padding, and so race, won't they?
> > I don't think we can clear padding on atomic_ref construction, only on
> > store and RMW operations.
>
> Didn't touch the above.
> >
> >
> > >--- a/libstdc++-v3/include/std/atomic
> > >+++ b/libstdc++-v3/include/std/atomic
>
> The patch against this file doesn't apply it all.
>
> > >--- /dev/null
> > >+++
> >
> b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc
> > >@@ -0,0 +1,43 @@
> > >+// { dg-options "-std=gnu++2a" }
> > >+// { dg-do run { target c++2a } }
> >
> > This new test is using "2a" not "20".
>
> Fixed thus, but the other testcase wasn't in the patch at all.
>
> Here it is:
>
> libstdc++: Clear padding bits in atomic compare_exchange
>
> This change implements P0528 which requires that padding bits not
> participate in atomic compare exchange operations. All arguments to the
> generic template are 'sanitized' by the __builtin_clear_padding intrinsic
> before they are used in comparisons. This requires that any stores
> also sanitize the incoming value.
>
> Signed-off-by: Thomas Rodgers 
>
> libstdc++-v3/ChangeLog:
>
> * include/std/atomic (atomic::atomic(_Tp)): Clear padding for
> __cplusplus > 201703L.
> (atomic::store()): Clear padding.
> (atomic::exchange()): Likewise.
> (atomic::compare_exchange_weak()): Likewise.
> (atomic::compare_exchange_strong()): Likewise.
> * include/bits/atomic_base.h (__atomic_impl::__clear_padding()):
> New function.
> (__atomic_impl::__maybe_has_padding()): Likewise.
> (__atomic_impl::__compare_exchange()): Likewise.
> (__atomic_impl::compare_exchange_weak()): Delegate to
> __compare_exchange().
> (__atomic_impl::compare_exchange_strong()): Likewise.
> * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New
> test.
> * testsuite/28_atomics/atomic_ref/compare_exchange_padding.cc:
> Likewise.
>
> --- a/libstdc++-v3/include/bits/atomic_base.h.jj2022-05-16
> 09:46:02.361059682 +0200
> +++ b/libstdc++-v3/include/bits/atomic_base.h   2022-08-25
> 12:06:13.758883172 +0200
> @@ -954,6 +954,87 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>/// @endcond
>
> +  // Implementation details of atomic padding handling
> +  namespace __atomic_impl
> +  {
> +template
> +  _GLIBCXX_ALWAYS_INLINE _Tp*
> +  __clear_padding(_Tp& __val) noexcept
> +  {
> +   auto* __ptr = std::__addressof(__val);
> +#if __has_builtin(__builtin_clear_padding)
> +   __builtin_clear_padding(std::__addressof(__val));
> +#endif
> +   return __ptr;
> +  }
> +
> +template
> +  constexpr bool
> +  __maybe_has_padding()
> +  {
> +#if ! __has_builtin(__builtin_clear_padding)
> +   return false;
> +#elif __has_builtin(__has_unique_object_representations)
> +   return !__has_unique_object_representations(_Tp)
> + && !is_floating_point<_Tp>::value;
> +#else
> +   return true;
> +#endif
> +  }
> +
> +template
> +  _GLIBCXX_ALWAYS_INLINE bool
> +  __compare_exchange(_Tp& __val, _Tp& __e, _Tp& __i, bool __weak,
> +memory_order __s, memory_order __f) noexcept
> +  {
> +   __glibcxx_assert(__is_valid_cmpexch_failure_order(__f));
> +
> +   if _GLIBCXX17_CONSTEXPR 

Re: Patch ping (Re: [PATCH] Implement __builtin_issignaling)

[Joseph Myers]

On Tue, 23 Aug 2022, Jakub Jelinek via Gcc-patches wrote:

> On Tue, Aug 16, 2022 at 11:41:06AM +, Richard Biener wrote:
> > I'm OK with the rest of the patch if Joseph doesn't have comments
> > on the actual issignaling lowerings (which I didn't review for
> > correctness due to lack of knowledge).
> 
> I'd like to ping this patch.
> Patch: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599697.html
> with
> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599794.html
> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599800.html
> incremental additions.

One testsuite comment:

> +#ifdef __SIZEOF_FLOAT128__
> +  if (f7 (w) || !f7 (__builtin_nansf128 ("0x123")) || f7 (42.0Q) || f7 
> (__builtin_nanf128 ("0x234"))
> +  || f7 (__builtin_inff128 ()) || f7 (-__builtin_inff128 ()) || f7 
> (-42.0Q) || f7 (-0.0Q) || f7 (0.0Q))
> +__builtin_abort ();

__SIZEOF_FLOAT128__ is a target-specific macro for two targets.  I think 
it's better to model things on the existing _FloatN and _FloatNx tests, so 
have such a test for each such type, preferably with most of the test 
implementation type-generic (see e.g. floatn-builtin.h) and then with the 
individual tests including e.g.

/* { dg-add-options float128 } */
/* { dg-require-effective-target float128_runtime } */

to enable and test for the relevant support.  That would mean _Float128 
gets tested wherever supported - and also that the support is properly 
tested for _Float16, which doesn't look like it's covered by the tests in 
this patch at all at present.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: Patch ping Re: [PATCH] i386: Fix up ix86_gimplify_va_arg [PR105331]

[Jeff Law via Gcc-patches]

On 4/28/2022 4:31 AM, Richard Biener wrote:

On Thu, 28 Apr 2022, Jakub Jelinek wrote:


On Thu, Apr 28, 2022 at 10:39:39AM +0200, Uros Bizjak wrote:

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2022-04-22  Jakub Jelinek  

   PR target/105331
   * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp
   temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR
   of it.

   * gcc.dg/pr105331.c: New test.

Sorry, I have no idea if this patch is correct or not.

Richard or Jeff, could you please review it instead?

OK.

Agreed.  In fact I'd fixed something very similar in our port a while ago.

Jeff

Re: Patch ping Re: [PATCH] i386: Fix up ix86_gimplify_va_arg [PR105331]

[Richard Biener via Gcc-patches]

On Thu, 28 Apr 2022, Jakub Jelinek wrote:

> On Thu, Apr 28, 2022 at 10:39:39AM +0200, Uros Bizjak wrote:
> > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > > >
> > > > 2022-04-22  Jakub Jelinek  
> > > >
> > > >   PR target/105331
> > > >   * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp
> > > >   temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR
> > > >   of it.
> > > >
> > > >   * gcc.dg/pr105331.c: New test.
> > 
> > Sorry, I have no idea if this patch is correct or not.
> 
> Richard or Jeff, could you please review it instead?

OK.

Thanks,
Richard.

Re: Patch ping Re: [PATCH] i386: Fix up ix86_gimplify_va_arg [PR105331]

[Jakub Jelinek via Gcc-patches]

On Thu, Apr 28, 2022 at 10:39:39AM +0200, Uros Bizjak wrote:
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > >
> > > 2022-04-22  Jakub Jelinek  
> > >
> > >   PR target/105331
> > >   * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp
> > >   temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR
> > >   of it.
> > >
> > >   * gcc.dg/pr105331.c: New test.
> 
> Sorry, I have no idea if this patch is correct or not.

Richard or Jeff, could you please review it instead?

Thanks.

Jakub

Re: Patch ping Re: [PATCH] i386: Fix up ix86_gimplify_va_arg [PR105331]

[Uros Bizjak via Gcc-patches]

On Thu, Apr 28, 2022 at 10:31 AM Jakub Jelinek  wrote:
>
> Hi!
>
> I'd like to ping this patch.  I know it isn't a full week yet, but we are
> almost out of P1s and GCC 12 branching is any time now.
>
> Thanks:
> On Fri, Apr 22, 2022 at 09:25:04AM +0200, Jakub Jelinek via Gcc-patches wrote:
> > On the following testcase we emit a bogus
> > 'va_arg_tmp.5' may be used uninitialized
> > warning.  The reason is that when gimplifying the addr = 
> > statement, the va_arg_tmp temporary var for which we emit ADDR_EXPR
> > is not TREE_ADDRESSABLE, prepare_gimple_addressable emits some extra
> > code to initialize the newly addressable var from its previous value,
> > but it is a new variable which hasn't been initialized yet and will
> > be later, so we end up initializing it with uninitialized SSA_NAME:
> >   va_arg_tmp.6 = va_arg_tmp.5_14(D);
> >   addr.2_16 = _arg_tmp.6;
> >   _17 = MEM[(double *)sse_addr.4_13];
> >   MEM[(double * {ref-all})addr.2_16] = _17;
> > and with -O1 we actually don't DSE it before the warning is emitted.
> > If we make the temp TREE_ADDRESSABLE before the gimplification, then
> > this prepare_gimple_addressable path isn't taken and we effectively
> > omit the first statement above and so the bogus warning is gone.
> >
> > I went through other backends and didn't find another instance of this
> > problem.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > 2022-04-22  Jakub Jelinek  
> >
> >   PR target/105331
> >   * config/i386/i386.cc (ix86_gimplify_va_arg): Mark va_arg_tmp
> >   temporary TREE_ADDRESSABLE before trying to gimplify ADDR_EXPR
> >   of it.
> >
> >   * gcc.dg/pr105331.c: New test.

Sorry, I have no idea if this patch is correct or not.

Uros.

Re: [PATCH PING] ipa: Careful processing ANCESTOR jump functions and NULL pointers (PR 103083)

[Martin Jambor]

Hello,

I would like to ping this patch, please.

Thanks,

Martin

On Mon, Feb 14 2022, Martin Jambor wrote:
> Hello Honza,
>
> On Mon, Dec 13 2021, Jan Hubicka wrote:
>>> >>> + || (only_for_nonzero && 
>>> >>> !src_lats->bits_lattice.known_nonzero_p ()))
>>> >>> +   {
>>> >>> + if (jfunc->bits)
>>> >>> +   return dest_lattice->meet_with (jfunc->bits->value,
>>> >>> +   jfunc->bits->mask, 
>>> >>> precision);
>>> >>> + else
>>> >>> +   return dest_lattice->set_to_bottom ();
>>> >>> +   }
>>> >> I do not think you need to set to bottom here. For pointers, we
>>> >> primarily track alignment and NULL is aligned - all you need to do is to
>>> >> make sure that we do not believe that some bits are 1.
>>> >
>>> > I am not sure I understand, the testcase you wrote has all bits as zeros
>>> > and still miscompiles?  It is primarily used for alignment but not only
>>> > for that.
>>
>> Maybe I misunderstand the code.  But if you have only_for_nonzero and
>> you do not know htat src lattice is non-zero you will get
>>  - if src is 0, then dest is 0
>>  - if src is non-zero then dest is src+offset
>>
>> So all trialing bits that are known to be 0 in src and offset are known
>> to be 0 in the result.  But I do not see where th eoffset is mixed in?
>>
>
> I went back and tried to figure out again what you meant and I hope it
> was the following patch, which does not drop the lattice to bottom for
> ANCESTORs but instead masks any bits that would be considered known
> ones.  It is indeed clever and I did not see the possibility when
> writing the patch.
>
> The following has passed bootstrap, LTO bootstrap and testing on
> x86-64.  OK for trunk (and then to be backported to 11 and 10)?
>
> Thanks,
>
> Martin
>
>
> IPA_JF_ANCESTOR jump functions are constructed also when the formal
> parameter of the caller is first checked whether it is NULL and left
> as it is if it is NULL, to accommodate C++ casts to an ancestor class.
>
> The jump function type was invented for devirtualization and IPA-CP
> propagation of tree constants is also careful to apply it only to
> existing DECLs(*) but as PR 103083 shows, the part propagating "known
> bits" was not careful about this, which can lead to miscompilations.
>
> This patch introduces a flag to the ancestor jump functions which
> tells whether a NULL-check was elided when creating it and makes the
> bits propagation behave accordingly, masking any bits otherwise would
> be known to be one.  This should safely preserve alignment info, which
> is the primary ifnormation that we keep in bits for pointers.
>
> (*) There still may remain problems when a DECL resides on address
> zero (with -fno-delete-null-pointer-checks ...I hope it cannot happen
> otherwise).  I am looking into that now but I think it will be easier
> for everyone if I do so in a follow-up patch.
>
> gcc/ChangeLog:
>
> 2022-02-11  Martin Jambor  
>
>   PR ipa/103083
>   * ipa-prop.h (ipa_ancestor_jf_data): New flag keep_null;
>   (ipa_get_jf_ancestor_keep_null): New function.
>   * ipa-prop.c (ipa_set_ancestor_jf): Initialize keep_null field of the
>   ancestor function.
>   (compute_complex_assign_jump_func): Pass false to keep_null
>   parameter of ipa_set_ancestor_jf.
>   (compute_complex_ancestor_jump_func): Pass true to keep_null
>   parameter of ipa_set_ancestor_jf.
>   (update_jump_functions_after_inlining): Carry over keep_null from the
>   original ancestor jump-function or merge them.
>   (ipa_write_jump_function): Stream keep_null flag.
>   (ipa_read_jump_function): Likewise.
>   (ipa_print_node_jump_functions_for_edge): Print the new flag.
>   * ipa-cp.c (class ipcp_bits_lattice): Make various getters const.  New
>   member function known_nonzero_p.
>   (ipcp_bits_lattice::known_nonzero_p): New.
>   (ipcp_bits_lattice::meet_with_1): New parameter drop_all_ones,
>   observe it.
>   (ipcp_bits_lattice::meet_with): Likewise.
>   (propagate_bits_across_jump_function): Simplify.  Pass true in
>   drop_all_ones when it is necessary.
>   (propagate_aggs_across_jump_function): Take care of keep_null
>   flag.
>   (ipa_get_jf_ancestor_result): Propagate NULL accross keep_null
>   jump functions.
>
> gcc/testsuite/ChangeLog:
>
> 2021-11-25  Martin Jambor  
>
>   * gcc.dg/ipa/pr103083-1.c: New test.
>   * gcc.dg/ipa/pr103083-2.c: Likewise.
> ---
>  gcc/ipa-cp.cc | 75 ++-
>  gcc/ipa-prop.cc   | 20 +--
>  gcc/ipa-prop.h| 13 +
>  gcc/testsuite/gcc.dg/ipa/pr103083-1.c | 28 ++
>  gcc/testsuite/gcc.dg/ipa/pr103083-2.c | 30 +++
>  5 files changed, 137 insertions(+), 29 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083-1.c
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103083-2.c
>
> diff 

Re: [PATCH PING] ipa-cp: Do not create clones for values outside known value range (PR 102513)

[Martin Jambor]

Hello,

I would like to ping this patch, please.

Thanks,

Martin


On Mon, Feb 14 2022, Martin Jambor wrote:
> Hi,
>
> PR 102513 shows we emit bogus array access warnings when IPA-CP
> creates clones specialized for values which it deduces from arithmetic
> jump functions describing self-recursive calls.  Those can however be
> avoided if we consult the IPA-VR information that the same pass also
> has.
>
> The patch below does that at the stage when normally values are only
> examined for profitability.  It would be better not to create lattices
> describing such bogus values in the first place, however that presents
> an ordering problem, the pass currently propagates all information,
> and so both constants and VR, in no particular order when processing
> SCCs, and so this approach seemed much simpler.
>
> I plan to rearrange the pass so that it clones in multiple passes over
> the call graph (or rather the lattice dependence graph) and it feels
> natural to only do propagation for these kinds of recursion in the
> second or later passes, which would fix the issue more elegantly.
>
> Bootstrapped and tested on x86_64, OK for trunk (and perhaps also for
> GCC 11)?
>
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2022-02-14  Martin Jambor  
>
>   PR ipa/102513
>   * ipa-cp.cc (decide_whether_version_node): Skip scalar values
>   which do not fit the known value_range.
>
> gcc/testsuite/ChangeLog:
>
> 2022-02-14  Martin Jambor  
>
>   PR ipa/102513
>   * gcc.dg/ipa/pr102513.c: New test.
> ---
>  gcc/ipa-cp.cc   | 28 ++--
>  gcc/testsuite/gcc.dg/ipa/pr102513.c | 33 +
>  2 files changed, 59 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr102513.c
>
> diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc
> index 453e9c93cc3..ec37632d487 100644
> --- a/gcc/ipa-cp.cc
> +++ b/gcc/ipa-cp.cc
> @@ -6154,8 +6154,32 @@ decide_whether_version_node (struct cgraph_node *node)
>   {
> ipcp_value *val;
> for (val = lat->values; val; val = val->next)
> - ret |= decide_about_value (node, i, -1, val, ,
> -_gen_clones);
> + {
> +   /* If some values generated for self-recursive calls with
> +  arithmetic jump functions fall outside of the known
> +  value_range for the parameter, we can skip them.  VR interface
> +  supports this only for integers now.  */
> +   if (TREE_CODE (val->value) == INTEGER_CST
> +   && !plats->m_value_range.bottom_p ()
> +   && !plats->m_value_range.m_vr.contains_p (val->value))
> + {
> +   /* This can happen also if a constant present in the source
> +  code falls outside of the range of parameter's type, so we
> +  cannot assert.  */
> +   if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> +   fprintf (dump_file, " - skipping%s value ",
> +val->self_recursion_generated_p ()
> +? " self_recursion_generated" : "");
> +   print_ipcp_constant_value (dump_file, val->value);
> +   fprintf (dump_file, " because it is outside known "
> +"value range.\n");
> + }
> +   continue;
> + }
> +   ret |= decide_about_value (node, i, -1, val, ,
> +  _gen_clones);
> + }
>   }
>  
>if (!plats->aggs_bottom)
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr102513.c 
> b/gcc/testsuite/gcc.dg/ipa/pr102513.c
> new file mode 100644
> index 000..9ee5431b730
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr102513.c
> @@ -0,0 +1,33 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -Warray-bounds" } */
> +
> +extern int block2[7][256];
> +
> +static int encode_block(int block2[7][256], unsigned level)
> +{
> +int best_score = 0;
> +
> +for (unsigned x = 0; x < level; x++) {
> +int v = block2[1][x];
> +block2[level][x] = 0;
> +best_score += v * v;
> +}
> +
> +if (level > 0 && best_score > 64) {
> +int score = 0;
> +
> +score += encode_block(block2, level - 1);
> +score += encode_block(block2, level - 1);
> +
> +if (score < best_score) {
> +best_score = score;
> +}
> +}
> +
> +return best_score;
> +}
> +
> +int foo(void)
> +{
> +return encode_block(block2, 5);
> +}
> -- 
> 2.34.1

Re: Patch ping (Re: [PATCH] libatomic: Improve 16-byte atomics on Intel AVX [PR104688])

[Uros Bizjak via Gcc-patches]

On Wed, Mar 16, 2022 at 6:50 PM Jakub Jelinek  wrote:
>
> Hi!
>
> I'd like to ping this patch:
> https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590960.html
>
> Thanks.
>
> On Mon, Feb 28, 2022 at 07:06:30AM +0100, Jakub Jelinek wrote:
> > As mentioned in the PR, the latest Intel SDM has added:
> > "Processors that enumerate support for Intel® AVX (by setting the feature 
> > flag CPUID.01H:ECX.AVX[bit 28])
> > guarantee that the 16-byte memory operations performed by the following 
> > instructions will always be
> > carried out atomically:
> > • MOVAPD, MOVAPS, and MOVDQA.
> > • VMOVAPD, VMOVAPS, and VMOVDQA when encoded with VEX.128.
> > • VMOVAPD, VMOVAPS, VMOVDQA32, and VMOVDQA64 when encoded with EVEX.128 and 
> > k0 (masking disabled).
> > (Note that these instructions require the linear addresses of their memory 
> > operands to be 16-byte
> > aligned.)"
> >
> > The following patch deals with it just on the libatomic library side so far,
> > currently (since ~ 2017) we emit all the __atomic_* 16-byte builtins as
> > library calls since and this is something that we can hopefully backport.
> >
> > The patch simply introduces yet another ifunc variant that takes priority
> > over the pure CMPXCHG16B one, one that checks AVX and CMPXCHG16B bits and
> > on non-Intel clears the AVX bit during detection for now (if AMD comes
> > with the same guarantee, we could revert the config/x86/init.c hunk),
> > which implements 16-byte atomic load as vmovdqa and 16-byte atomic store
> > as vmovdqa followed by mfence.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk so far?
> >
> > 2022-02-28  Jakub Jelinek  
> >
> >   PR target/104688
> >   * Makefile.am (IFUNC_OPTIONS): Change on x86_64 to -mcx16 -mcx16.
> >   (libatomic_la_LIBADD): Add $(addsuffix _16_2_.lo,$(SIZEOBJS)) for
> >   x86_64.
> >   * Makefile.in: Regenerated.
> >   * config/x86/host-config.h (IFUNC_COND_1): For x86_64 define to
> >   both AVX and CMPXCHG16B bits.
> >   (IFUNC_COND_2): Define.
> >   (IFUNC_NCOND): For x86_64 define to 2 * (N == 16).
> >   (MAYBE_HAVE_ATOMIC_CAS_16, MAYBE_HAVE_ATOMIC_EXCHANGE_16,
> >   MAYBE_HAVE_ATOMIC_LDST_16): Define to IFUNC_COND_2 rather than
> >   IFUNC_COND_1.
> >   (HAVE_ATOMIC_CAS_16): Redefine to 1 whenever IFUNC_ALT != 0.
> >   (HAVE_ATOMIC_LDST_16): Redefine to 1 whenever IFUNC_ALT == 1.
> >   (atomic_compare_exchange_n): Define whenever IFUNC_ALT != 0
> >   on x86_64 for N == 16.
> >   (__atomic_load_n, __atomic_store_n): Redefine whenever IFUNC_ALT == 1
> >   on x86_64 for N == 16.
> >   (atomic_load_n, atomic_store_n): New functions.
> >   * config/x86/init.c (__libat_feat1_init): On x86_64 clear bit_AVX
> >   if CPU vendor is not Intel.

LGTM.

Thanks,
Uros.

Re: Patch ping

[Jeff Law via Gcc-patches]

On 3/2/2022 2:47 AM, Jakub Jelinek wrote:

Hi!

I'd like to ping the

https://gcc.gnu.org/pipermail/gcc-patches/2022-February/590526.html
PR104558 - when bypassing emit_push_insn for 0 sized arg, emit at least 
anti_adjust_stack for alignment pad if needed

patch.
So the issue is the stack isn't aligned to the pad, thus triggering the 
abort.  Right?  Assuming that's the case, the patch is OK with a 
suitable comment.


Jeff

Re: Patch ping

[Jan Hubicka via Gcc-patches]

> Hi!
> 
> I'd like to ping the
> https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586553.html
>   symtab: Fold  ==  to 0 if folding_initializer [PR94716]
> 
> patch.  Thanks.
OK.
Note that with LTO partitioning it may happen that alias is defined in
one partition but used in another.  We do take care to bring the symtab
info with it so it should be safe.

Honza
> 
>   Jakub
> 

Re: Patch ping

[Richard Biener via Gcc-patches]

On Mon, 3 Jan 2022, Jakub Jelinek wrote:

> Hi!
> 
> I'd like to ping the
> https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586553.html
>   symtab: Fold  ==  to 0 if folding_initializer [PR94716]

OK.

Thanks,
Richard.

Re: Patch ping

[Richard Biener via Gcc-patches]

On Mon, 3 Jan 2022, Jakub Jelinek wrote:

> Hi!
> 
> I'd like to ping the middle-end part of the
> https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586879.html
> patch (which Uros approved the backend part for with a minor change
> I have in my tree).

OK for the middle-end parts if you properly amend md.texi

Richard.

Re: Patch ping related to OpenMP

[Tobias Burnus]

Thanks for a DWARF-related patch review (+ fix by yourself). Otherwise,
still pending are the following OpenMP patches.

The first one is a revised patch following the review comment and
affects Fortran only. The second one is also a rather small &
post-review revised patch.

On 06.12.21 15:56, Tobias Burnus wrote:

First, thanks for the four reviews. Secondly, I missed one patch –
hence, reposted with all three pending patches:

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584894.html

and:

On 01.12.21 17:34, Tobias Burnus wrote:

* [PATCH] C, C++, Fortran, OpenMP: Add 'has_device_addr' clause to
'target' construct
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585361.html

* [PATCH 00/16] OpenMP: lvalues in "map" clauses and struct handling
rework
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585439.html

Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955

Re: Patch ping related to OpenMP

[Tobias Burnus]

First, thanks for the four reviews. Secondly, I missed one patch –
hence, reposted with all three pending patches:

* Re: [PATCH] [gfortran] Add support for allocate clause (OpenMP 5.0).
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584894.html

and:

On 01.12.21 17:34, Tobias Burnus wrote:

* [PATCH] C, C++, Fortran, OpenMP: Add 'has_device_addr' clause to
'target' construct
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585361.html

* [PATCH 00/16] OpenMP: lvalues in "map" clauses and struct handling
rework
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/585439.html


Tobias

PS: Thanks for the reviews of the following patches:

* [PATCH, v5, OpenMP 5.0] Improve OpenMP target support for C++ [PR92120
v5]
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584602.html

* [PATCH, v2, OpenMP 5.0] Remove array section base-pointer mapping
semantics, and other front-end adjustments (mainline trunk)
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584994.html

* libgomp.texi: Update OMP_PLACES
https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582121.html

* [patch] Fortran/OpenMP: Support most of 5.1 atomic extensions
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584450.html

-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955

Re: Patch ping (Re: [PATCH] x86_64: Issue -Wpsabi warning about C++ zero width bitfield ABI changes [PR102024])

[H.J. Lu via Gcc-patches]

On Mon, Nov 29, 2021 at 3:24 AM Jakub Jelinek  wrote:
>
> Hi!
>
> On Tue, Aug 31, 2021 at 10:05:58AM +0200, Jakub Jelinek via Gcc-patches wrote:
> > This is an incremental patch to
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578447.html
> > for x86_64 ABI.
> > For zero-width bitfields current GCC classify_argument does:
> >   if (DECL_BIT_FIELD (field))
> > {
> >   for (i = (int_bit_position (field)
> > + (bit_offset % 64)) / 8 / 8;
> >i < ((int_bit_position (field) + (bit_offset % 
> > 64))
> > + tree_to_shwi (DECL_SIZE (field))
> > + 63) / 8 / 8; i++)
> > classes[i]
> >   = merge_classes (X86_64_INTEGER_CLASS, 
> > classes[i]);
> > }
> > which I think means that if the zero-width bitfields are at bit-positions
> > (in the toplevel aggregate) which are multiples of 64 bits doesn't do
> > anything, (int_bit_position (field) + (bit_offset % 64)) / 64 and
> > (int_bit_position (field) + (bit_offset % 64) + 63) / 64 should be equal.
> > But for zero-width bitfields at other bit positions it will call
> > merge_classes once.  Now, the typical case is that the zero width bitfield
> > is surrounded by some bitfields and in that case, it doesn't change
> > anything, but it can be sandwitched in between floats too as the testcases
> > show.
> > In C we had this behavior, in C++ previously the FE was removing the
> > zero-width bitfields and therefore they were ignored.
> > LLVM and ICC seems to ignore those bitfields both in C and C++ (== passing
> > struct S in SSE register rather than in GPR).
>
> I'd like to ping this patch, but perhaps first it would be nice to discuss
> it in the x86-64 psABI group.
> The current psABI doesn't seem to mention zero sized bitfields at all
> explicitly, so perhaps theoretically they should be treated as INTEGER class,
> but if they are at positions multiple of 64 bits, then it is unclear into
> which eightbyte they should be considered, whether the previous one if any
> or the next one if any.  I guess similar problem is for zero sized
> structures, but those should according to algorithm have NO_CLASS and so it
> doesn't really make a difference.  And, no compiler I'm aware of treats
> the zero sized bitfields at 64 bit boundaries as INTEGER class, LLVM/ICC are
> ignoring such bitfields everywhere, GCC ignores them at those boundaries
> (and used to ignore them in C++ everywhere).  I guess my preferred solution
> would be to say explicitly that zero sized bitfields are NO_CLASS.
> I'm not a member of the google x86-64 psABI group, can somebody please raise
> it there?

https://groups.google.com/g/x86-64-abi/c/OYeWs14WHQ4

> > The following patch assumes the current GCC C behavior of not ignoring them
> > when not at bitpositions divisible by 64 is right (though I'm really not
> > sure about that) and implements a -Wpsabi warning for that case.
> > The psABI IMHO should be clarified in any case.
> > The other option is to start ignoring them always (and issue -Wpsabi warning
> > if !DECL_FIELD_ABI_IGNORED on zero-width bitfield and it would change the
> > outcome).
> >
> > I think libffi doesn't need changing, as I think it doesn't support
> > bitfields.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux.
> >
> > 2021-08-31  Jakub Jelinek  
> >
> >   PR target/102024
> >   * config/i386/i386.c (classify_argument): Add cxx_bitfields argument,
> >   when seeing DECL_FIELD_ABI_IGNORED bitfields either set it to 1 or
> >   if equal to 2 ignore it.  Pass it to recursive calls.  Add wrapper
> >   with old arguments and diagnose ABI differences for C++ structures
> >   with zero width bitfields.  Formatting fixes.
> >
> >   * gcc.target/i386/pr102024.c: New test.
> >   * g++.target/i386/pr102024.C: New test.
>
> Jakub
>


-- 
H.J.

Re: Patch ping (was Re: [PATCH] rs6000: Fix up easy_vector_constant_msb handling [PR101384])

[Segher Boessenkool]

Hi!

On Tue, Jul 20, 2021 at 02:43:03PM +0200, Jakub Jelinek wrote:
> For gcc 11, I've bootstrapped/regtested on powerpc64le-linux and
> powerpc64-linux (the latter regtested -m32/-m64) also a simpler version
> below, which restricts it to the case that the code handles properly.
> 
> 2021-07-20  Jakub Jelinek  
> 
>   PR target/101384
>   * config/rs6000/rs6000.c (vspltis_constant): Accept EASY_VECTOR_MSB
>   only if step and copies are equal to 1.
> 
>   * gcc.dg/pr101384.c: New test.

Okay for all backports.  Thanks!


Segher

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Andrew Sutton via Gcc-patches]

> Is just using std::terminate as the handler viable? Or if we're sure
> contracts in some form will go into the IS eventually, and the
> signature won't change, we could just add it in __cxxabiv1:: as you
> suggested earlier.

No, the handler needs to be configurable (at least quietly) in order
to support experimentation by SG21. No idea if it will stay that way.

Andrew

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jonathan Wakely via Gcc-patches]

On Wed, 14 Jul 2021 at 04:56, Jason Merrill  wrote:
>
> On 7/12/21 3:58 PM, Jonathan Wakely wrote:
> > On Mon, 5 Jul 2021 at 20:07, Jason Merrill  wrote:
> >>
> >> On 6/26/21 10:23 AM, Andrew Sutton wrote:
> >>>
> >>> I ended up taking over this work from Jeff (CC'd on his existing email
> >>> address). I scraped all the contracts changes into one big patch
> >>> against master. See attached. The ChangeLog.contracts files list the
> >>> sum of changes for the patch, not the full history of the work.
> >>
> >> Jonathan, can you advise where the library support should go?
> >>
> >> In N4820  was part of the language-support clause, which makes
> >> sense, but it uses string_view, which brings in a lot of the rest of the
> >> library.  Did LWG talk about this when contracts went in?  How are
> >> freestanding implementations expected to support contracts?
> >
> > I don't recall that being discussed, but I think I was in another room
> > for much of the contracts review.
> >
> > If necessary we could make the std::char_traits specialization
> > available freestanding, without the primary template (or the other
> > specializations). But since C++20 std::string_view also depends on
> > quite a lot of ranges, which depends on iterators, which is not
> > freestanding. Some of those dependencies were added more recently than
> > contracts was reviewed and then yanked out, so maybe wasn't considered
> > a big problem back then. In any case, depending on std::string_view
> > (even without the rest of std::basic_string_view) is not currently
> > possible for freestanding.
>
> I guess I'll change string_view to const char* for now.

I think that's best. Making std::string_view usable would take some work.

> >> I imagine the header should be  for now.
> >
> > Agreed.
>
> And the type std::experimental::??::contract_violation.  Maybe
> contracts_v1 for the inline namespace?

LGTM

> Did you have any thoughts about the violation handler?  Is it OK to add
> a default definition to the library, in the above namespace?

I'd rather not have any std::experimental::* symbols go into the DSO.
For std::experimental::filesystem we added libstdc++fs.a, with no
corresponding .so library, which users need to link to explicitly to
use that TS. Would something like libstdc++contracts.a work here? Is
it just one symbol?

Aside: Ulrich Drepper suggested recently that the driver should have
been updated to automatically add -lstdc++fs so that using
 was seamless, as the archive contents
wouldn't be used unless something in the program referred to the
symbols in it.

Is just using std::terminate as the handler viable? Or if we're sure
contracts in some form will go into the IS eventually, and the
signature won't change, we could just add it in __cxxabiv1:: as you
suggested earlier.

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jason Merrill via Gcc-patches]

On 7/12/21 3:58 PM, Jonathan Wakely wrote:

On Mon, 5 Jul 2021 at 20:07, Jason Merrill  wrote:


On 6/26/21 10:23 AM, Andrew Sutton wrote:


I ended up taking over this work from Jeff (CC'd on his existing email
address). I scraped all the contracts changes into one big patch
against master. See attached. The ChangeLog.contracts files list the
sum of changes for the patch, not the full history of the work.


Jonathan, can you advise where the library support should go?

In N4820  was part of the language-support clause, which makes
sense, but it uses string_view, which brings in a lot of the rest of the
library.  Did LWG talk about this when contracts went in?  How are
freestanding implementations expected to support contracts?


I don't recall that being discussed, but I think I was in another room
for much of the contracts review.

If necessary we could make the std::char_traits specialization
available freestanding, without the primary template (or the other
specializations). But since C++20 std::string_view also depends on
quite a lot of ranges, which depends on iterators, which is not
freestanding. Some of those dependencies were added more recently than
contracts was reviewed and then yanked out, so maybe wasn't considered
a big problem back then. In any case, depending on std::string_view
(even without the rest of std::basic_string_view) is not currently
possible for freestanding.


I guess I'll change string_view to const char* for now.


I imagine the header should be  for now.


Agreed.


And the type std::experimental::??::contract_violation.  Maybe 
contracts_v1 for the inline namespace?


Did you have any thoughts about the violation handler?  Is it OK to add 
a default definition to the library, in the above namespace?


Jason

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jonathan Wakely via Gcc-patches]

On Mon, 5 Jul 2021 at 20:07, Jason Merrill  wrote:
>
> On 6/26/21 10:23 AM, Andrew Sutton wrote:
> >
> > I ended up taking over this work from Jeff (CC'd on his existing email
> > address). I scraped all the contracts changes into one big patch
> > against master. See attached. The ChangeLog.contracts files list the
> > sum of changes for the patch, not the full history of the work.
>
> Jonathan, can you advise where the library support should go?
>
> In N4820  was part of the language-support clause, which makes
> sense, but it uses string_view, which brings in a lot of the rest of the
> library.  Did LWG talk about this when contracts went in?  How are
> freestanding implementations expected to support contracts?

I don't recall that being discussed, but I think I was in another room
for much of the contracts review.

If necessary we could make the std::char_traits specialization
available freestanding, without the primary template (or the other
specializations). But since C++20 std::string_view also depends on
quite a lot of ranges, which depends on iterators, which is not
freestanding. Some of those dependencies were added more recently than
contracts was reviewed and then yanked out, so maybe wasn't considered
a big problem back then. In any case, depending on std::string_view
(even without the rest of std::basic_string_view) is not currently
possible for freestanding.

> I imagine the header should be  for now.

Agreed.

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jason Merrill via Gcc-patches]

On 7/5/21 3:07 PM, Jason Merrill wrote:

On 6/26/21 10:23 AM, Andrew Sutton wrote:


I ended up taking over this work from Jeff (CC'd on his existing email
address). I scraped all the contracts changes into one big patch
against master. See attached. The ChangeLog.contracts files list the
sum of changes for the patch, not the full history of the work.


Jonathan, can you advise where the library support should go?

In N4820  was part of the language-support clause, which makes 
sense, but it uses string_view, which brings in a lot of the rest of the 
library.  Did LWG talk about this when contracts went in?  How are 
freestanding implementations expected to support contracts?


I imagine the header should be  for now.

You've previously mentioned that various current experimental features 
don't appear in libstdc++.so; that is not true of the current patch.


I see that https://github.com/arcosuc3m/clang-contracts takes the 
approach, of teaching the compiler about std::contract_violation, 
building up an object, and passing it to the handler directly, much like 
we do for initializer_list.  Their equivalent of __on_contract_violation 
is an internal function emitted in each translation unit that needs it, 
so it doesn't need to affect the library ABI.  These both seem like 
improvements to me.


More complicated is the question of the default violation handler: the 
lock3 implementation calls it "handle_contract_violation" in the global 
namespace, and overriding it is done with ELF symbol interposition, much 
like the replaceable allocation functions.  That approach seems 
reasonable, but I'd think we should use a reserved name, e.g. 
::__handle_contract_violation or __cxxabiv1::__contract_violation_handler.


The clang implementation above involves specifying the name of the 
handler on the compiler command line, which seems problematic, as it 
would tend to mean multiple independent violation handlers active at the 
same time.  Their default handler is std::terminate, which does avoid 
needing to add the default handler to the library.


I've pushed the patch with my adjustments so far to devel/c++-contracts 
on gcc.gnu.org.  One of those adjustments was changing the 
contract_violation data members to be const char *, to make creating an 
object easier, and making the member functions inline, to reduce the 
number of symbols added to the library.


Jason

contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jason Merrill via Gcc-patches]

On 6/26/21 10:23 AM, Andrew Sutton wrote:


I ended up taking over this work from Jeff (CC'd on his existing email
address). I scraped all the contracts changes into one big patch
against master. See attached. The ChangeLog.contracts files list the
sum of changes for the patch, not the full history of the work.


Jonathan, can you advise where the library support should go?

In N4820  was part of the language-support clause, which makes 
sense, but it uses string_view, which brings in a lot of the rest of the 
library.  Did LWG talk about this when contracts went in?  How are 
freestanding implementations expected to support contracts?


I imagine the header should be  for now.

You've previously mentioned that various current experimental features 
don't appear in libstdc++.so; that is not true of the current patch.


I see that https://github.com/arcosuc3m/clang-contracts takes the 
approach, of teaching the compiler about std::contract_violation, 
building up an object, and passing it to the handler directly, much like 
we do for initializer_list.  Their equivalent of __on_contract_violation 
is an internal function emitted in each translation unit that needs it, 
so it doesn't need to affect the library ABI.  These both seem like 
improvements to me.


More complicated is the question of the default violation handler: the 
lock3 implementation calls it "handle_contract_violation" in the global 
namespace, and overriding it is done with ELF symbol interposition, much 
like the replaceable allocation functions.  That approach seems 
reasonable, but I'd think we should use a reserved name, e.g. 
::__handle_contract_violation or __cxxabiv1::__contract_violation_handler.


The clang implementation above involves specifying the name of the 
handler on the compiler command line, which seems problematic, as it 
would tend to mean multiple independent violation handlers active at the 
same time.  Their default handler is std::terminate, which does avoid 
needing to add the default handler to the library.


Jason

Re: [PATCH] PING implement pre-c++20 contracts

[Andrew Sutton via Gcc-patches]

I think so, yes.

On Fri, Jul 2, 2021 at 11:09 AM Jason Merrill  wrote:
>
> On 7/1/21 12:27 PM, Andrew Sutton wrote:
> >>> I think this version addresses most of your concerns.
> >>
> >> Thanks, looking good.  I'll fuss with it a bit and commit it soon.
>
> Do you agree that this testcase should compile?

Re: [PATCH] PING implement pre-c++20 contracts

[Jason Merrill via Gcc-patches]

On 7/1/21 12:27 PM, Andrew Sutton wrote:

I think this version addresses most of your concerns.


Thanks, looking good.  I'll fuss with it a bit and commit it soon.


Do you agree that this testcase should compile?
>From 85400e1896a188892b1ebeb0c8e86ff3cd28cfa6 Mon Sep 17 00:00:00 2001
From: Jason Merrill 
Date: Wed, 30 Jun 2021 16:57:44 -0400
Subject: [PATCH] assume-cx
To: gcc-patches@gcc.gnu.org

---
 gcc/cp/constexpr.c| 26 +++
 .../g++.dg/contracts/contracts-constexpr3.C   | 10 +++
 2 files changed, 31 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/contracts/contracts-constexpr3.C

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 435bf530d68..66b3ccce713 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -7022,12 +7022,26 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
 	  break;
 
 	tree c = CONTRACT_CONDITION (t);
-	if (semantic == CCS_ASSUME && !cp_tree_defined_p (c))
-	  break;
+	if (semantic == CCS_ASSUME)
+	  {
+	/* For an assume contract, try evaluating it without instantiating
+	   anything.  If non-constant, assume it's satisfied.  */
 
-	/* Evaluate the generated check.  */
-	r = cxx_eval_constant_expression (ctx, c, false, non_constant_p,
-	  overflow_p);
+	if (!cp_tree_defined_p (c))
+	  break;
+
+	bool dummy_nc = false, dummy_ov = false;
+	constexpr_ctx new_ctx = *ctx;
+	new_ctx.quiet = true;
+	r = cxx_eval_constant_expression (_ctx, c, false,
+	  _nc, _ov);
+	if (dummy_nc)
+	  break;
+	  }
+	else
+	  /* Evaluate the generated check.  */
+	  r = cxx_eval_constant_expression (ctx, c, false, non_constant_p,
+	overflow_p);
 	if (r == boolean_false_node)
 	  {
 	if (!ctx->quiet)
@@ -8948,6 +8962,8 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
 case ASSERTION_STMT:
 case PRECONDITION_STMT:
 case POSTCONDITION_STMT:
+  if (!checked_contract_p (get_contract_semantic (t)))
+	return true;
   if (!RECUR (CONTRACT_CONDITION (t), rval))
 	return false;
   return true;
diff --git a/gcc/testsuite/g++.dg/contracts/contracts-constexpr3.C b/gcc/testsuite/g++.dg/contracts/contracts-constexpr3.C
new file mode 100644
index 000..8826220ef91
--- /dev/null
+++ b/gcc/testsuite/g++.dg/contracts/contracts-constexpr3.C
@@ -0,0 +1,10 @@
+// An assumed contract shouldn't break constant evaluation.
+
+// { dg-do compile { target c++20 } }
+// { dg-additional-options -fcontracts }
+
+bool b;
+
+constexpr int f() [[ pre assume: b ]] { return 42; }
+
+static_assert (f() > 0);
-- 
2.27.0

Re: [PATCH] PING implement pre-c++20 contracts

[Andrew Sutton via Gcc-patches]

> > I think this version addresses most of your concerns.
>
> Thanks, looking good.  I'll fuss with it a bit and commit it soon.

Awesome!

Andrew

Re: [PATCH] PING implement pre-c++20 contracts

[Jason Merrill via Gcc-patches]

On 6/26/21 10:23 AM, Andrew Sutton wrote:

Hi Jason,

I ended up taking over this work from Jeff (CC'd on his existing email
address). I scraped all the contracts changes into one big patch
against master. See attached. The ChangeLog.contracts files list the
sum of changes for the patch, not the full history of the work.

I think this version addresses most of your concerns.


Thanks, looking good.  I'll fuss with it a bit and commit it soon.


There are a few big changes.

The first is that we treat contracts like any other attribute, which
gets interesting at times. For example, in duplicate_decl, we have to
do a little dance to make sure the target merge_attributes doesn't
copy attributes between the new and old decls in seemingly arbitrary
order. Friends are also a bit funny because the attributes aren't
attached by cplus_decl_attributes at the point declarations are
merged, so we have to defer comparisons.

Contracts are always parsed where they appear, except on member
functions. For postconditions with result variables (e.g., [[post r:
...]]), we temporarily declare r as if 'auto r' and then update it
later when we've computed the function's return type. (I feel like
this was kind of overlooked in N4820... it's generally impossible to
assign a type to 'r' given the position of contract attributes in the
declarator: 'auto f(int n) [[post r: q]] -> int'. It's worse in GCC
since the return type is computed in grokdeclarator, well after
contract attributes have been parsed).

On a related note, the handling of postconditions involving deduced
return type was completely rewritten. Everything happens in
apply_deduced_return_type, which seems right. I think
check_return_expr is where the postcondition is actually invoked.

We no longer instantiate contract attributes until absolutely
necessary in regenerate_decl_from_tempalte. That seems to work well...
at least does after I discovered we were quietly rewriting contract
lists every time we removed contracts from an old declaration or from
a template specialization. This also gets rid of the need to have
unshare_templates, which had a FIXME note attached.

Lastly, we only ever generate pre/post checks for actual functions,
not function templates. I also simplified a lot of the logic around
associating pre/post check functions, so that it's only set exactly
once when start analyzing function definitions.

I believe Jeff addressed some of the ABI concerns and COMDAT-related questions.

On the issue of copy_fn_decl vs.v copy_fndecl_with_name... I didn't
change that. The latter sends the function to middle end for codegen,
which we really don't want at the point we make the copy.

I think we're probably still breaking NRVO. I didn't get a chance to
look at that.



On Fri, May 28, 2021 at 9:18 AM Jeff Chapman  wrote:


Hello again :)  Wanted to shoot a quick status update. Some github issues have
been created for points of feedback, and we've been working on addressing them.
A few changes have been pushed to the contracts-jac-alt branch, while there's
also an active more in depth rewrite branch. Some specific comments inline
below.

On 5/17/21, Jason Merrill  wrote:

On 5/14/21 4:54 PM, Jason Merrill wrote:

On 4/30/21 1:44 PM, Jeff Chapman wrote:

Hello! Looping back around to this. re:
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567334.html

On 3/25/21, Jason Merrill  wrote:

On 3/1/21 8:12 AM, Jeff Chapman wrote:

On 1/18/21, Jason Merrill  wrote:

On 1/4/21 9:58 AM, Jeff Chapman wrote:

Ping. re:
https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html


https://github.com/lock3/gcc/tree/contracts-jac-alt



Why is some of the code in c-family?  From the modules merge there is
now a cp_handle_option function that you could add the option
handling
to, and I don't see a reason for cxx-contracts.c to be in c-family/
rather than cp/.


I've been pushing changes that address the points raised and wanted to
ping to see if there's more feedback and give a status summary. The
notable change is making sure the implementation plays nicely with
modules and a mangling change that did away with a good chunk of code.

The contracts code outside of cp/ has been moved into it, and the
contract attributes have been altered to work with language
independent
handling code. More of the contracts code can probably still be
moved to
cxx-contracts which I'll loop back to after other refactoring. The
naming, spelling, and comments (or lack thereof) have been addressed.


Sounds good.  I plan to get back to this when GCC 11 branches, which
should be mid-April.


Please let me know if you see any more issues when you pick it back up.
Particularly in modules interop, since I don't think you've had a chance
to look at that yet.

Completed another merge with master earlier this week which didn't bring
to light any new issues or regressions, but 

Re: [PATCH] PING implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

Hello again :)  Wanted to shoot a quick status update. Some github issues have
been created for points of feedback, and we've been working on addressing them.
A few changes have been pushed to the contracts-jac-alt branch, while there's
also an active more in depth rewrite branch. Some specific comments inline
below.

On 5/17/21, Jason Merrill  wrote:
> On 5/14/21 4:54 PM, Jason Merrill wrote:
>> On 4/30/21 1:44 PM, Jeff Chapman wrote:
>>> Hello! Looping back around to this. re:
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567334.html
>>>
>>> On 3/25/21, Jason Merrill  wrote:
 On 3/1/21 8:12 AM, Jeff Chapman wrote:
> On 1/18/21, Jason Merrill  wrote:
>> On 1/4/21 9:58 AM, Jeff Chapman wrote:
>>> Ping. re:
>>> https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html
>>> 
>>>
>>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>>> 
>>>
>> Why is some of the code in c-family?  From the modules merge there is
>> now a cp_handle_option function that you could add the option
>> handling
>> to, and I don't see a reason for cxx-contracts.c to be in c-family/
>> rather than cp/.
>
> I've been pushing changes that address the points raised and wanted to
> ping to see if there's more feedback and give a status summary. The
> notable change is making sure the implementation plays nicely with
> modules and a mangling change that did away with a good chunk of code.
>
> The contracts code outside of cp/ has been moved into it, and the
> contract attributes have been altered to work with language
> independent
> handling code. More of the contracts code can probably still be
> moved to
> cxx-contracts which I'll loop back to after other refactoring. The
> naming, spelling, and comments (or lack thereof) have been addressed.

 Sounds good.  I plan to get back to this when GCC 11 branches, which
 should be mid-April.
>>>
>>> Please let me know if you see any more issues when you pick it back up.
>>> Particularly in modules interop, since I don't think you've had a chance
>>> to look at that yet.
>>>
>>> Completed another merge with master earlier this week which didn't bring
>>> to light any new issues or regressions, but I'll keep on that :)
>>>
>>> +  /* If we have contracts, check that they're valid in this
>>> context. */
>>> +  // FIXME: These aren't entirely correct.
>>
>> How not?  Can local extern function decls have contract attributes?
>>
>>> + /* FIXME when we instatiate a template class with
>>> guarded
>>> +  * members, particularly guarded template members,
>>> the resulting
>>> +  * pre/post functions end up being inaccessible
>>> because their
>>> +  * template info's context is the original
>>> uninstantiated class.
>>
>> This sounds like a significant functionality gap.  I'm guessing you
>> want
>> to reconsider the unshare_template approach.
>>
>> One approach would be to only do the pre/post/guarded/unguarded
>> transformation for a fully-instantiated function; a temploid function
>> would leave the contracts as attributes.
>>

There's an in progress rewrite which moves pre/post function generation until
much later which should resolve this. Like you suggested, these are not created
until we're completely out of template processing.

>>> +  /* FIXME do we need magic to perfectly forward this so we
>>> don't clobber
>>> +RVO/NRVO etc?  */
>>
>> Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you
>> want.
>
> These points are still being investigated and addressed; including
> them
> for reference.
>>
>> Any update?
>>

CALL_FROM_THUNK_P is being set now when an actual call is built. Is there a good
way to test that the optimization isn't being broken in general?

>> More soon.
>
> Please let me know what other issues need work.
>>>
>>> If there's anything I can do to make the process smoother please don't
>>> hesitate to ask.
>>
>> Larger-scope comments:
>>
>> Please add an overview of the implementation strategy to the top of
>> cxx-contracts.c.  Particularly to discuss the why and how of
>> pre/post/guarded/unguarded functions.

An initial overview has been added, though it'll need updated with some of the
pending rewrites.

>
>> I'm confused by the approach to late parsing of contracts; it seems like
>> you wait until the end of parsing the function to go back and parse the
>> contracts.  Why not parse them sooner, along with nsdmis, noexcept, and
>> function bodies?

Parsing has been moved forward on the rewrite branch.

>>
>> Smaller-scope comments:
>>
 +   /* If we didn't build a check, insert a NOP so we don't leak
 + 

Re: [PATCH] PING implement pre-c++20 contracts

[Jason Merrill via Gcc-patches]

On 5/14/21 4:54 PM, Jason Merrill wrote:

On 4/30/21 1:44 PM, Jeff Chapman wrote:

Hello! Looping back around to this. re:
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567334.html

On 3/25/21, Jason Merrill  wrote:

On 3/1/21 8:12 AM, Jeff Chapman wrote:

On 1/18/21, Jason Merrill  wrote:

On 1/4/21 9:58 AM, Jeff Chapman wrote:

Ping. re:
https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html


https://github.com/lock3/gcc/tree/contracts-jac-alt



Why is some of the code in c-family?  From the modules merge there is
now a cp_handle_option function that you could add the option handling
to, and I don't see a reason for cxx-contracts.c to be in c-family/
rather than cp/.


I've been pushing changes that address the points raised and wanted to
ping to see if there's more feedback and give a status summary. The
notable change is making sure the implementation plays nicely with
modules and a mangling change that did away with a good chunk of code.

The contracts code outside of cp/ has been moved into it, and the
contract attributes have been altered to work with language independent
handling code. More of the contracts code can probably still be 
moved to

cxx-contracts which I'll loop back to after other refactoring. The
naming, spelling, and comments (or lack thereof) have been addressed.


Sounds good.  I plan to get back to this when GCC 11 branches, which
should be mid-April.


Please let me know if you see any more issues when you pick it back up.
Particularly in modules interop, since I don't think you've had a chance
to look at that yet.

Completed another merge with master earlier this week which didn't bring
to light any new issues or regressions, but I'll keep on that :)

+  /* If we have contracts, check that they're valid in this 
context. */

+  // FIXME: These aren't entirely correct.


How not?  Can local extern function decls have contract attributes?

+ /* FIXME when we instatiate a template class with 
guarded
+  * members, particularly guarded template members, 
the resulting
+  * pre/post functions end up being inaccessible 
because their
+  * template info's context is the original 
uninstantiated class.


This sounds like a significant functionality gap.  I'm guessing you 
want

to reconsider the unshare_template approach.


One approach would be to only do the pre/post/guarded/unguarded 
transformation for a fully-instantiated function; a temploid function 
would leave the contracts as attributes.


+  /* FIXME do we need magic to perfectly forward this so we 
don't clobber

+    RVO/NRVO etc?  */


Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you
want.


These points are still being investigated and addressed; including them
for reference.


Any update?


More soon.


Please let me know what other issues need work.


If there's anything I can do to make the process smoother please don't
hesitate to ask.


Larger-scope comments:

Please add an overview of the implementation strategy to the top of 
cxx-contracts.c.  Particularly to discuss the why and how of 
pre/post/guarded/unguarded functions.


I'm confused by the approach to late parsing of contracts; it seems like 
you wait until the end of parsing the function to go back and parse the 
contracts.  Why not parse them sooner, along with nsdmis, noexcept, and 
function bodies?


Smaller-scope comments:


+   /* If we didn't build a check, insert a NOP so we don't leak
+  contracts into GENERIC.  */
+   *stmt_p = build1 (NOP_EXPR, void_type_node, integer_zero_node);


You can use void_node for the NOP.


+  error_at (EXPR_LOCATION (new_contract),
+   "mismatched contract condition in %s",
+   ctx == cmc_declaration ? "declaration" : "override");


This sort of build-up of diagnostic messages by substring replacement 
doesn't work very well for translations.  In general, don't use %s for 
inserting English text, only code strings that will be the same in all 
languages.



+  /* Remove the associated contracts and unchecked result, if any.  */
+  if (flag_contracts && TREE_CODE (newdecl) == FUNCTION_DECL)
+    {
+  remove_contract_attributes (newdecl);
+  set_contract_functions (newdecl, NULL_TREE, NULL_TREE);
+    }


Why bother removing attributes on a decl that's about to be freed?

Why did we set the contract functions above only to clear them now?


   if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
  /* Aliases are definitions. */
- && !alias)
+ && !alias
+ && (DECL_VIRTUAL_P (decl) || !flag_contracts))
    permerror (declarator->id_loc,
   "declaration of %q#D outside of class is not 
definition",

   decl);
+  else if (DECL_EXTERNAL (decl) && ! 

Re: [PATCH] PING implement pre-c++20 contracts

[Marek Polacek via Gcc-patches]

On Fri, May 14, 2021 at 04:54:10PM -0400, Jason Merrill via Gcc-patches wrote:
> Please add an overview of the implementation strategy to the top of
> cxx-contracts.c.  Particularly to discuss the why and how of
> pre/post/guarded/unguarded functions.

And I think let's please name the file contracts.cc.

Marek

Re: [PATCH] PING implement pre-c++20 contracts

[Jason Merrill via Gcc-patches]

On 4/30/21 1:44 PM, Jeff Chapman wrote:

Hello! Looping back around to this. re:
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567334.html

On 3/25/21, Jason Merrill  wrote:

On 3/1/21 8:12 AM, Jeff Chapman wrote:

On 1/18/21, Jason Merrill  wrote:

On 1/4/21 9:58 AM, Jeff Chapman wrote:

Ping. re:
https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html


https://github.com/lock3/gcc/tree/contracts-jac-alt



Why is some of the code in c-family?  From the modules merge there is
now a cp_handle_option function that you could add the option handling
to, and I don't see a reason for cxx-contracts.c to be in c-family/
rather than cp/.


I've been pushing changes that address the points raised and wanted to
ping to see if there's more feedback and give a status summary. The
notable change is making sure the implementation plays nicely with
modules and a mangling change that did away with a good chunk of code.

The contracts code outside of cp/ has been moved into it, and the
contract attributes have been altered to work with language independent
handling code. More of the contracts code can probably still be moved to
cxx-contracts which I'll loop back to after other refactoring. The
naming, spelling, and comments (or lack thereof) have been addressed.


Sounds good.  I plan to get back to this when GCC 11 branches, which
should be mid-April.


Please let me know if you see any more issues when you pick it back up.
Particularly in modules interop, since I don't think you've had a chance
to look at that yet.

Completed another merge with master earlier this week which didn't bring
to light any new issues or regressions, but I'll keep on that :)


+  /* If we have contracts, check that they're valid in this context. */
+  // FIXME: These aren't entirely correct.


How not?  Can local extern function decls have contract attributes?


+ /* FIXME when we instatiate a template class with guarded
+  * members, particularly guarded template members, the resulting
+  * pre/post functions end up being inaccessible because their
+  * template info's context is the original uninstantiated class.


This sounds like a significant functionality gap.  I'm guessing you want
to reconsider the unshare_template approach.


One approach would be to only do the pre/post/guarded/unguarded 
transformation for a fully-instantiated function; a temploid function 
would leave the contracts as attributes.



+  /* FIXME do we need magic to perfectly forward this so we don't clobber
+RVO/NRVO etc?  */


Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you
want.


These points are still being investigated and addressed; including them
for reference.


Any update?


More soon.


Please let me know what other issues need work.


If there's anything I can do to make the process smoother please don't
hesitate to ask.


Larger-scope comments:

Please add an overview of the implementation strategy to the top of 
cxx-contracts.c.  Particularly to discuss the why and how of 
pre/post/guarded/unguarded functions.


I'm confused by the approach to late parsing of contracts; it seems like 
you wait until the end of parsing the function to go back and parse the 
contracts.  Why not parse them sooner, along with nsdmis, noexcept, and 
function bodies?


Smaller-scope comments:


+   /* If we didn't build a check, insert a NOP so we don't leak
+  contracts into GENERIC.  */
+   *stmt_p = build1 (NOP_EXPR, void_type_node, integer_zero_node);


You can use void_node for the NOP.


+  error_at (EXPR_LOCATION (new_contract),
+   "mismatched contract condition in %s",
+   ctx == cmc_declaration ? "declaration" : "override");


This sort of build-up of diagnostic messages by substring replacement 
doesn't work very well for translations.  In general, don't use %s for 
inserting English text, only code strings that will be the same in all 
languages.



+  /* Remove the associated contracts and unchecked result, if any.  */
+  if (flag_contracts && TREE_CODE (newdecl) == FUNCTION_DECL)
+{
+  remove_contract_attributes (newdecl);
+  set_contract_functions (newdecl, NULL_TREE, NULL_TREE);
+}


Why bother removing attributes on a decl that's about to be freed?

Why did we set the contract functions above only to clear them now?


   if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
  /* Aliases are definitions. */
- && !alias)
+ && !alias
+ && (DECL_VIRTUAL_P (decl) || !flag_contracts))
permerror (declarator->id_loc,
   "declaration of %q#D outside of class is not definition",
   decl);
+  else if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
+ /* Aliases are definitions. */

Re: Patch ping (Re: [PATCH] ix86: Support V{2, 4}DImode arithmetic right shifts for SSE2+ [PR98856])

[Uros Bizjak via Gcc-patches]

On Wed, May 12, 2021 at 3:06 PM Jakub Jelinek  wrote:
>
> On Fri, Apr 23, 2021 at 03:46:59PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > On Tue, Feb 09, 2021 at 12:12:24PM +0100, Jakub Jelinek via Gcc-patches 
> > wrote:
> > > As mentioned in the PR, we don't support arithmetic right V2DImode or
> > > V4DImode on x86 without -mavx512vl or -mxop.  The ISAs indeed don't have
> > > {,v}psraq instructions until AVX512VL, but we actually can emulate it 
> > > quite
> > > easily.
> > > One case is arithmetic >> 63, we can just emit {,v}pxor; {,v}pcmpgt for
> > > that for SSE4.2+, or for SSE2 psrad $31; pshufd $0xf5.
> > > Then arithmetic >> by constant > 32, that can be done with {,v}psrad $31
> > > and {,v}psrad $(cst-32) and two operand permutation,
> > > arithmetic >> 32 can be done as {,v}psrad $31 and permutation of that
> > > and the original operand.  Arithmetic >> by constant < 32 can be done
> > > as {,v}psrad $cst and {,v}psrlq $cst and two operand permutation.
> > > And arithmetic >> by variable scalar amount can be done as
> > > arithmetic >> 63, logical >> by the amount, << by (64 - amount of the
> > > >> 63 result; note that the vector << 64 result in 0) and oring together.
> > >
> > > I had to improve the permutation generation so that it actually handles
> > > the needed permutations (or handles them better).
> > >
> > > If not, I guess this is a GCC 12 material.
> > >
> > > 2021-02-09  Jakub Jelinek  
> > >
> > > PR tree-optimization/98856
> > > * config/i386/i386.c (ix86_shift_rotate_cost): Add CODE argument.
> > > Expect V2DI and V4DI arithmetic right shifts to be emulated.
> > > (ix86_rtx_costs, ix86_add_stmt_cost): Adjust ix86_shift_rotate_cost
> > > caller.
> > > * config/i386/i386-expand.c (expand_vec_perm_2perm_interleave,
> > > expand_vec_perm_2perm_pblendv): New functions.
> > > (ix86_expand_vec_perm_const_1): Use them.
> > > * config/i386/sse.md (ashr3): Rename to ...
> > > (ashr3): ... this.
> > > (ashr3): New define_expand with VI248_AVX512BW iterator.
> > > (ashrv4di3): New define_expand.
> > > (ashrv2di3): Change condition to TARGET_SSE2, handle !TARGET_XOP
> > > and !TARGET_AVX512VL expansion.
> > >
> > > * gcc.target/i386/sse2-psraq-1.c: New test.
> > > * gcc.target/i386/sse4_2-psraq-1.c: New test.
> > > * gcc.target/i386/avx-psraq-1.c: New test.
> > > * gcc.target/i386/avx2-psraq-1.c: New test.
> > > * gcc.target/i386/avx-pr82370.c: Adjust expected number of vpsrad
> > > instructions.
> > > * gcc.target/i386/avx2-pr82370.c: Likewise.
> > > * gcc.target/i386/avx512f-pr82370.c: Likewise.
> > > * gcc.target/i386/avx512bw-pr82370.c: Likewise.
> > > * gcc.dg/torture/vshuf-4.inc: Add two further permutations.
> > > * gcc.dg/torture/vshuf-8.inc: Likewise.
> >
> > Ok for trunk now?
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565026.html
>
> I'd like to ping this patch.

LGTM.

Thanks,
Uros.

Re: [PATCH] PING implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

Hello! Looping back around to this. re:
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567334.html

On 3/25/21, Jason Merrill  wrote:
> On 3/1/21 8:12 AM, Jeff Chapman wrote:
>> On 1/18/21, Jason Merrill  wrote:
>>> On 1/4/21 9:58 AM, Jeff Chapman wrote:
 Ping. re:
 https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html
 

 https://github.com/lock3/gcc/tree/contracts-jac-alt
 

>>> Why is some of the code in c-family?  From the modules merge there is
>>> now a cp_handle_option function that you could add the option handling
>>> to, and I don't see a reason for cxx-contracts.c to be in c-family/
>>> rather than cp/.
>>
>> I've been pushing changes that address the points raised and wanted to
>> ping to see if there's more feedback and give a status summary. The
>> notable change is making sure the implementation plays nicely with
>> modules and a mangling change that did away with a good chunk of code.
>>
>> The contracts code outside of cp/ has been moved into it, and the
>> contract attributes have been altered to work with language independent
>> handling code. More of the contracts code can probably still be moved to
>> cxx-contracts which I'll loop back to after other refactoring. The
>> naming, spelling, and comments (or lack thereof) have been addressed.
>
> Sounds good.  I plan to get back to this when GCC 11 branches, which
> should be mid-April.

Please let me know if you see any more issues when you pick it back up.
Particularly in modules interop, since I don't think you've had a chance
to look at that yet.

Completed another merge with master earlier this week which didn't bring
to light any new issues or regressions, but I'll keep on that :)

 +  /* If we have contracts, check that they're valid in this context. */
 +  // FIXME: These aren't entirely correct.
>>>
>>> How not?  Can local extern function decls have contract attributes?
>>>
 + /* FIXME when we instatiate a template class with guarded
 +  * members, particularly guarded template members, the 
 resulting
 +  * pre/post functions end up being inaccessible because their
 +  * template info's context is the original uninstantiated 
 class.
>>>
>>> This sounds like a significant functionality gap.  I'm guessing you want
>>> to reconsider the unshare_template approach.
>>>
 +  /* FIXME do we need magic to perfectly forward this so we don't 
 clobber
 +RVO/NRVO etc?  */
>>>
>>> Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you
>>> want.
>>
>> These points are still being investigated and addressed; including them
>> for reference.
>>
>>> More soon.
>>
>> Please let me know what other issues need work.

If there's anything I can do to make the process smoother please don't
hesitate to ask.

Thank you,
Jeff Chapman

Re: Patch ping for PR95176 fix

[Jeff Law via Gcc-patches]

On 4/12/2021 11:45 AM, Victor Tong via Gcc-patches wrote:

Hello,

I'd like to ping this patch. It contains two new tree-opt patterns in match.pd.

[PATCH] tree-optimization: Optimize division followed by multiply [PR95176] 
(gnu.org)


We're currently working towards the gcc-11 release, in particular we're 
only fixing regression bugs at this point.  So this patch will be 
re-considered once gcc-12 development opens, hopefully within the next 
few weeks.



jeff

Re: Patch ping

[Richard Biener]

On Wed, 31 Mar 2021, Jakub Jelinek wrote:

> Hi!
> 
> I'd like to ping the PR98860 P1 fix - workaround for PECOFF linkers without
> DWARF5 support - to make -gdwarf-4 the default in such configurations.
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567245.html

OK.

Richard.

> Thanks
> 
>   Jakub
> 
> 

-- 
Richard Biener 
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Re: [PATCH] PING implement pre-c++20 contracts

[Jason Merrill via Gcc-patches]

On 3/1/21 8:12 AM, Jeff Chapman wrote:

On 1/18/21, Jason Merrill  wrote:

On 1/4/21 9:58 AM, Jeff Chapman wrote:

Ping. re:
https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html


https://github.com/lock3/gcc/tree/contracts-jac-alt



Why is some of the code in c-family?  From the modules merge there is
now a cp_handle_option function that you could add the option handling
to, and I don't see a reason for cxx-contracts.c to be in c-family/
rather than cp/.


I've been pushing changes that address the points raised and wanted to
ping to see if there's more feedback and give a status summary. The
notable change is making sure the implementation plays nicely with
modules and a mangling change that did away with a good chunk of code.

The contracts code outside of cp/ has been moved into it, and the
contract attributes have been altered to work with language independent
handling code. More of the contracts code can probably still be moved to
cxx-contracts which I'll loop back to after other refactoring. The
naming, spelling, and comments (or lack thereof) have been addressed.


Sounds good.  I plan to get back to this when GCC 11 branches, which 
should be mid-April.



+set_decl_contracts (tree decl, tree contract_attrs)

I think you want to use 'chainon' here.



+build_arg_list (tree fn)

You can use is_this_parameter here.


Thanks; I knew of chainon but didn't think of it and is_this_parameter
is new to me. Always fun to delete code :)


+  /* If we have contracts, check that they're valid in this context.  */
+  // FIXME: These aren't entirely correct.


How not?  Can local extern function decls have contract attributes?


+ /* FIXME when we instatiate a template class with guarded
+  * members, particularly guarded template members, the resulting
+  * pre/post functions end up being inaccessible because their
+  * template info's context is the original uninstantiated class.


This sounds like a significant functionality gap.  I'm guessing you want
to reconsider the unshare_template approach.


+  /* FIXME do we need magic to perfectly forward this so we don't clobber
+RVO/NRVO etc?  */


Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you want.


These points are still being investigated and addressed; including them
for reference.


More soon.


Please let me know what other issues need work.


Thank you!
Jeff Chapman

Re: Patch ping

[Richard Biener]

On Wed, 24 Mar 2021, Martin Sebor wrote:

> On 3/24/21 10:40 AM, Jakub Jelinek wrote:
> > On Wed, Mar 24, 2021 at 09:45:31AM -0600, Martin Sebor via Gcc-patches
> > wrote:
> >> E.g., OEP_IGNORE_MEMBER_OFFSET or OEP_SAME_MEMBER_OFFSET (for
> >> the converse of the first) or something like that, but hopefully
> >> you get the idea.
> > 
> > Neither of these look like a good name to me, OEP_IGNORE_MEMBER_OFFSET
> > seems like a request that member offset is not important to the equality,
> > it is always important, but in the new mode we want not just the member
> > offset to be equal, but also the fields to be the same, i.e. a stronger
> > requirement.
> > 
> > So, what about
> >/* For OEP_ADDRESS_OF of COMPONENT_REFs, only consider same fields as
> >   equivalent rather than also different fields with the same offset.  */
> >OEP_ADDRESS_OF_SAME_FIELD = 256
> 
> This name works for me.

Works for me as well, aka OK.

Richard.

> Thanks
> Martin
> 
> > 
> >  Jakub
> > 
> 
> 
> 

-- 
Richard Biener 
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Re: Patch ping

[Martin Sebor via Gcc-patches]

On 3/24/21 10:40 AM, Jakub Jelinek wrote:

On Wed, Mar 24, 2021 at 09:45:31AM -0600, Martin Sebor via Gcc-patches wrote:

E.g., OEP_IGNORE_MEMBER_OFFSET or OEP_SAME_MEMBER_OFFSET (for
the converse of the first) or something like that, but hopefully
you get the idea.


Neither of these look like a good name to me, OEP_IGNORE_MEMBER_OFFSET
seems like a request that member offset is not important to the equality,
it is always important, but in the new mode we want not just the member
offset to be equal, but also the fields to be the same, i.e. a stronger
requirement.

So, what about
   /* For OEP_ADDRESS_OF of COMPONENT_REFs, only consider same fields as
  equivalent rather than also different fields with the same offset.  */
   OEP_ADDRESS_OF_SAME_FIELD = 256


This name works for me.

Thanks
Martin



Jakub

Re: Patch ping

[Jakub Jelinek via Gcc-patches]

On Wed, Mar 24, 2021 at 09:45:31AM -0600, Martin Sebor via Gcc-patches wrote:
> E.g., OEP_IGNORE_MEMBER_OFFSET or OEP_SAME_MEMBER_OFFSET (for
> the converse of the first) or something like that, but hopefully
> you get the idea.

Neither of these look like a good name to me, OEP_IGNORE_MEMBER_OFFSET
seems like a request that member offset is not important to the equality,
it is always important, but in the new mode we want not just the member
offset to be equal, but also the fields to be the same, i.e. a stronger
requirement.

So, what about
  /* For OEP_ADDRESS_OF of COMPONENT_REFs, only consider same fields as
 equivalent rather than also different fields with the same offset.  */
  OEP_ADDRESS_OF_SAME_FIELD = 256

Jakub

Re: Patch ping

[Jeff Law via Gcc-patches]

On 3/24/2021 5:44 AM, Jakub Jelinek via Gcc-patches wrote:

Hi!

I'd like to ping the
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566821.html
P1 PR99565 fix.

Marek has acked the gcc/c/ and gcc/c-family/ part of that patch, but it still
has gcc/cp/ and gcc/ parts that weren't acked.
If you have suggestions for better OEP_* flag name, I can change it.
Once Martin and you agree on a name, this is fine with the name update 
[if any].


Jeff

Re: Patch ping

[Martin Sebor via Gcc-patches]

On 3/24/21 5:44 AM, Jakub Jelinek via Gcc-patches wrote:

Hi!

I'd like to ping the
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566821.html
P1 PR99565 fix.

Marek has acked the gcc/c/ and gcc/c-family/ part of that patch, but it still
has gcc/cp/ and gcc/ parts that weren't acked.
If you have suggestions for better OEP_* flag name, I can change it.



Yes, as I said, I think changing it would be helpful.  I already
tried to make some these points in my comments on the patch but
it might help to reiterate them.

A good API name reflects the purpose of the API rather than one of
its (conceivably many) use cases.

By way of an example, when a new flag was needed to implement
-Wduplicate-branches it wasn't called OEM_DUPLICATE_BRANCHES but
OEM_LEXICOGRAPHIC.  That was a good choice of a name because it's
generic enough not to be surprising when used in other contexts.

For the same reason, when an another flag is needed to refine
the behavior of the function in a way that affects the same warning,
it too should describe the effect of the flag on the function rather
than just one possible use case.

Since the new flag determines whether or not distinct members at
the same offset are considered equal, a name that mentions MEMBER
and OFFSET might be suitable.

E.g., OEP_IGNORE_MEMBER_OFFSET or OEP_SAME_MEMBER_OFFSET (for
the converse of the first) or something like that, but hopefully
you get the idea.

Martin

Re: [PATCH PING^3] Add input_modes parameter to TARGET_MD_ASM_ADJUST hook

[Jeff Law via Gcc-patches]

On 3/3/21 1:42 PM, Ilya Leoshkevich wrote:
> On Wed, 2021-03-03 at 21:26 +0100, Ilya Leoshkevich via Gcc-patches
> wrote:
>> On Wed, 2021-03-03 at 13:02 -0700, Jeff Law wrote:
>>>
>>> On 3/2/21 4:50 PM, Ilya Leoshkevich via Gcc-patches wrote:
 Hello,

 I would like to ping the following patch:

 Add input_modes parameter to TARGET_MD_ASM_ADJUST hook
  https://gcc.gnu.org/pipermail/gcc-patches/2021-January/562898.html

 It is needed for the following regression fix:

 IBM Z: Fix usage of "f" constraint with long doubles
  https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564380.html


 Jakub, who would be the right person to review this change?  I've
 decided to ask you, since `git shortlog -ns gcc/cfgexpand.c` shows
 that
 you deal with this code a lot.

 Best regards,
 Ilya




 If TARGET_MD_ASM_ADJUST changes a mode of an input operand (which
 should be ok as long as the hook itself as well as after_md_seq
 make up
 for it), input_mode will contain stale information.

 It might be tempting to fix this by removing input_mode altogether
 and
 just using GET_MODE (), but this will not work correctly with
 constants.
 So add input_modes parameter and document that it should be updated
 whenever inputs parameter is updated.

 gcc/ChangeLog:

 2021-01-05  Ilya Leoshkevich  

 * cfgexpand.c (expand_asm_loc): Pass new parameter.
 (expand_asm_stmt): Likewise.
 * config/arm/aarch-common-protos.h (arm_md_asm_adjust): Add
 new
 parameter.
 * config/arm/aarch-common.c (arm_md_asm_adjust): Likewise.
 * config/arm/arm.c (thumb1_md_asm_adjust): Likewise.
 * config/cris/cris.c (cris_md_asm_adjust): Likewise.
 * config/i386/i386.c (ix86_md_asm_adjust): Likewise.
 * config/mn10300/mn10300.c (mn10300_md_asm_adjust):
 Likewise.
 * config/nds32/nds32.c (nds32_md_asm_adjust): Likewise.
 * config/pdp11/pdp11.c (pdp11_md_asm_adjust): Likewise.
 * config/rs6000/rs6000.c (rs6000_md_asm_adjust): Likewise.
 * config/vax/vax.c (vax_md_asm_adjust): Likewise.
 * config/visium/visium.c (visium_md_asm_adjust): Likewise.
 * target.def (md_asm_adjust): Likewise.
>>> Ugh.    A couple questions
>>> Are there any cases where you're going to want to change modes for
>>> arguments that were constants?   I'm a bit surprised that we don't
>>> have
>>> a mode for constants for the cases that we care about.  Presumably we
>>> can get a (modeless) CONST_INT here and we're not restricted to
>>> CONST_DOUBLE and friends (which have modes).
>> Yes, this might happen.  For example, here:
>>
>>     asm("sqxbr\t%0,%1" : "=f"(res) : "f"(0x1.1p+0L));
>>
>> the (const_double) and the corresponding operand will initially have 
>> the mode TFmode.  s390_md_asm_adjust () will add a conversion from
>> TFmode to FPRX2mode and change the argument accordingly.
> Just to be more precise: the mode of the (const_double) itself will not
> change.  Here is the resulting RTL for the asm statement above:
>
> # s390_md_asm_adjust () step 1: put the (const_double) operand into a
> # new (reg) with the same mode
> (insn (set (reg:TF 63)
>(const_double:TF ...)))
OK, but you still need to know the constant's original mode so that you
can get the right  mode on the insn above and in the SUBREG_REG for the
insn in step #2.  Those cases were exposed by the testsuite.

I'd originally wondered if we could just not adjust in the CONST_INT
case, but I think I was mis-understanding the implementation of
s390_md_asm_adjust in an important way.

I also think I focused too much on expand_asm_loc where it doesn't look
like we read input_mode after the call to MD_ASM_ADJUST.   But we
certainly do in expand_asm_stmt.


OK for the trunk.

Thanks,
jeff

Re: [PATCH PING^3] Add input_modes parameter to TARGET_MD_ASM_ADJUST hook

[Ilya Leoshkevich via Gcc-patches]

On Wed, 2021-03-03 at 21:26 +0100, Ilya Leoshkevich via Gcc-patches
wrote:
> On Wed, 2021-03-03 at 13:02 -0700, Jeff Law wrote:
> > 
> > 
> > On 3/2/21 4:50 PM, Ilya Leoshkevich via Gcc-patches wrote:
> > > Hello,
> > > 
> > > I would like to ping the following patch:
> > > 
> > > Add input_modes parameter to TARGET_MD_ASM_ADJUST hook
> > >  https://gcc.gnu.org/pipermail/gcc-patches/2021-January/562898.html
> > > 
> > > It is needed for the following regression fix:
> > > 
> > > IBM Z: Fix usage of "f" constraint with long doubles
> > >  https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564380.html
> > > 
> > > 
> > > Jakub, who would be the right person to review this change?  I've
> > > decided to ask you, since `git shortlog -ns gcc/cfgexpand.c` shows
> > > that
> > > you deal with this code a lot.
> > > 
> > > Best regards,
> > > Ilya
> > > 
> > > 
> > > 
> > > 
> > > If TARGET_MD_ASM_ADJUST changes a mode of an input operand (which
> > > should be ok as long as the hook itself as well as after_md_seq
> > > make up
> > > for it), input_mode will contain stale information.
> > > 
> > > It might be tempting to fix this by removing input_mode altogether
> > > and
> > > just using GET_MODE (), but this will not work correctly with
> > > constants.
> > > So add input_modes parameter and document that it should be updated
> > > whenever inputs parameter is updated.
> > > 
> > > gcc/ChangeLog:
> > > 
> > > 2021-01-05  Ilya Leoshkevich  
> > > 
> > > * cfgexpand.c (expand_asm_loc): Pass new parameter.
> > > (expand_asm_stmt): Likewise.
> > > * config/arm/aarch-common-protos.h (arm_md_asm_adjust): Add
> > > new
> > > parameter.
> > > * config/arm/aarch-common.c (arm_md_asm_adjust): Likewise.
> > > * config/arm/arm.c (thumb1_md_asm_adjust): Likewise.
> > > * config/cris/cris.c (cris_md_asm_adjust): Likewise.
> > > * config/i386/i386.c (ix86_md_asm_adjust): Likewise.
> > > * config/mn10300/mn10300.c (mn10300_md_asm_adjust):
> > > Likewise.
> > > * config/nds32/nds32.c (nds32_md_asm_adjust): Likewise.
> > > * config/pdp11/pdp11.c (pdp11_md_asm_adjust): Likewise.
> > > * config/rs6000/rs6000.c (rs6000_md_asm_adjust): Likewise.
> > > * config/vax/vax.c (vax_md_asm_adjust): Likewise.
> > > * config/visium/visium.c (visium_md_asm_adjust): Likewise.
> > > * target.def (md_asm_adjust): Likewise.
> > Ugh.    A couple questions
> > Are there any cases where you're going to want to change modes for
> > arguments that were constants?   I'm a bit surprised that we don't
> > have
> > a mode for constants for the cases that we care about.  Presumably we
> > can get a (modeless) CONST_INT here and we're not restricted to
> > CONST_DOUBLE and friends (which have modes).
> 
> Yes, this might happen.  For example, here:
> 
>     asm("sqxbr\t%0,%1" : "=f"(res) : "f"(0x1.1p+0L));
> 
> the (const_double) and the corresponding operand will initially have 
> the mode TFmode.  s390_md_asm_adjust () will add a conversion from
> TFmode to FPRX2mode and change the argument accordingly.

Just to be more precise: the mode of the (const_double) itself will not
change.  Here is the resulting RTL for the asm statement above:

# s390_md_asm_adjust () step 1: put the (const_double) operand into a
# new (reg) with the same mode
(insn (set (reg:TF 63)
   (const_double:TF ...)))

# s390_md_asm_adjust () step 2: convert a reg from TFmode to FPRX2mode
(insn (set (reg:FPRX2 65)
   (subreg:FPRX2 (reg:TF 63) 0)))

# s390_md_asm_adjust () step 3: replace the original operand with the
# resulting (reg), adjust (asm_input) accordingly
(insn (set (reg:FPRX2 64)
   (asm_operands:FPRX2 ("sqxbr %0,%1") ("=f") 0
   [(reg:FPRX2 65)]
   [(asm_input:FPRX2 ("f"))])))

Re: [PATCH PING^3] Add input_modes parameter to TARGET_MD_ASM_ADJUST hook

[Ilya Leoshkevich via Gcc-patches]

On Wed, 2021-03-03 at 13:02 -0700, Jeff Law wrote:
> 
> 
> On 3/2/21 4:50 PM, Ilya Leoshkevich via Gcc-patches wrote:
> > Hello,
> > 
> > I would like to ping the following patch:
> > 
> > Add input_modes parameter to TARGET_MD_ASM_ADJUST hook
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-January/562898.html
> > 
> > It is needed for the following regression fix:
> > 
> > IBM Z: Fix usage of "f" constraint with long doubles
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564380.html
> > 
> > 
> > Jakub, who would be the right person to review this change?  I've
> > decided to ask you, since `git shortlog -ns gcc/cfgexpand.c` shows
> > that
> > you deal with this code a lot.
> > 
> > Best regards,
> > Ilya
> > 
> > 
> > 
> > 
> > If TARGET_MD_ASM_ADJUST changes a mode of an input operand (which
> > should be ok as long as the hook itself as well as after_md_seq
> > make up
> > for it), input_mode will contain stale information.
> > 
> > It might be tempting to fix this by removing input_mode altogether
> > and
> > just using GET_MODE (), but this will not work correctly with
> > constants.
> > So add input_modes parameter and document that it should be updated
> > whenever inputs parameter is updated.
> > 
> > gcc/ChangeLog:
> > 
> > 2021-01-05  Ilya Leoshkevich  
> > 
> > * cfgexpand.c (expand_asm_loc): Pass new parameter.
> > (expand_asm_stmt): Likewise.
> > * config/arm/aarch-common-protos.h (arm_md_asm_adjust): Add
> > new
> > parameter.
> > * config/arm/aarch-common.c (arm_md_asm_adjust): Likewise.
> > * config/arm/arm.c (thumb1_md_asm_adjust): Likewise.
> > * config/cris/cris.c (cris_md_asm_adjust): Likewise.
> > * config/i386/i386.c (ix86_md_asm_adjust): Likewise.
> > * config/mn10300/mn10300.c (mn10300_md_asm_adjust):
> > Likewise.
> > * config/nds32/nds32.c (nds32_md_asm_adjust): Likewise.
> > * config/pdp11/pdp11.c (pdp11_md_asm_adjust): Likewise.
> > * config/rs6000/rs6000.c (rs6000_md_asm_adjust): Likewise.
> > * config/vax/vax.c (vax_md_asm_adjust): Likewise.
> > * config/visium/visium.c (visium_md_asm_adjust): Likewise.
> > * target.def (md_asm_adjust): Likewise.
> Ugh.    A couple questions
> Are there any cases where you're going to want to change modes for
> arguments that were constants?   I'm a bit surprised that we don't
> have
> a mode for constants for the cases that we care about.  Presumably we
> can get a (modeless) CONST_INT here and we're not restricted to
> CONST_DOUBLE and friends (which have modes).

Yes, this might happen.  For example, here:

asm("sqxbr\t%0,%1" : "=f"(res) : "f"(0x1.1p+0L));

the (const_double) and the corresponding operand will initially have 
the mode TFmode.  s390_md_asm_adjust () will add a conversion from
TFmode to FPRX2mode and change the argument accordingly.

However, this is not the problematic case that I refer to in the
commit message:  I caught some failures in the testsuite that I
tracked down to (const_int)s, which, like you mentioned, don't have
a mode.

> Is input_modes read after the call to md_asm_adjust?   I'm trying to
> figure out why we'd need to update it.

Yes, its contents goes into (asm_operand)'s (asm_input). If we don't
adjust it, (asm_input)s will no longer be consistent with input operand
RTXes.

> Not acking or naking at this point, I just want to make sure I
> understand what's going on.
> 
> jeff

Re: [PATCH PING^3] Add input_modes parameter to TARGET_MD_ASM_ADJUST hook

[Jeff Law via Gcc-patches]

On 3/2/21 4:50 PM, Ilya Leoshkevich via Gcc-patches wrote:
> Hello,
>
> I would like to ping the following patch:
>
> Add input_modes parameter to TARGET_MD_ASM_ADJUST hook
> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/562898.html
>
> It is needed for the following regression fix:
>
> IBM Z: Fix usage of "f" constraint with long doubles
> https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564380.html
>
>
> Jakub, who would be the right person to review this change?  I've
> decided to ask you, since `git shortlog -ns gcc/cfgexpand.c` shows that
> you deal with this code a lot.
>
> Best regards,
> Ilya
>
>
>
>
> If TARGET_MD_ASM_ADJUST changes a mode of an input operand (which
> should be ok as long as the hook itself as well as after_md_seq make up
> for it), input_mode will contain stale information.
>
> It might be tempting to fix this by removing input_mode altogether and
> just using GET_MODE (), but this will not work correctly with constants.
> So add input_modes parameter and document that it should be updated
> whenever inputs parameter is updated.
>
> gcc/ChangeLog:
>
> 2021-01-05  Ilya Leoshkevich  
>
>   * cfgexpand.c (expand_asm_loc): Pass new parameter.
>   (expand_asm_stmt): Likewise.
>   * config/arm/aarch-common-protos.h (arm_md_asm_adjust): Add new
>   parameter.
>   * config/arm/aarch-common.c (arm_md_asm_adjust): Likewise.
>   * config/arm/arm.c (thumb1_md_asm_adjust): Likewise.
>   * config/cris/cris.c (cris_md_asm_adjust): Likewise.
>   * config/i386/i386.c (ix86_md_asm_adjust): Likewise.
>   * config/mn10300/mn10300.c (mn10300_md_asm_adjust): Likewise.
>   * config/nds32/nds32.c (nds32_md_asm_adjust): Likewise.
>   * config/pdp11/pdp11.c (pdp11_md_asm_adjust): Likewise.
>   * config/rs6000/rs6000.c (rs6000_md_asm_adjust): Likewise.
>   * config/vax/vax.c (vax_md_asm_adjust): Likewise.
>   * config/visium/visium.c (visium_md_asm_adjust): Likewise.
>   * target.def (md_asm_adjust): Likewise.
Ugh.    A couple questions
Are there any cases where you're going to want to change modes for
arguments that were constants?   I'm a bit surprised that we don't have
a mode for constants for the cases that we care about.  Presumably we
can get a (modeless) CONST_INT here and we're not restricted to
CONST_DOUBLE and friends (which have modes).

Is input_modes read after the call to md_asm_adjust?   I'm trying to
figure out why we'd need to update it.

Not acking or naking at this point, I just want to make sure I
understand what's going on.

jeff

Re: [PATCH] PING implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

On 1/18/21, Jason Merrill  wrote:
> On 1/4/21 9:58 AM, Jeff Chapman wrote:
>> Ping. re:
>> https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html
>> 
>>
>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>> 
>>
> Why is some of the code in c-family?  From the modules merge there is
> now a cp_handle_option function that you could add the option handling
> to, and I don't see a reason for cxx-contracts.c to be in c-family/
> rather than cp/.

I've been pushing changes that address the points raised and wanted to
ping to see if there's more feedback and give a status summary. The
notable change is making sure the implementation plays nicely with
modules and a mangling change that did away with a good chunk of code.

The contracts code outside of cp/ has been moved into it, and the
contract attributes have been altered to work with language independent
handling code. More of the contracts code can probably still be moved to
cxx-contracts which I'll loop back to after other refactoring. The
naming, spelling, and comments (or lack thereof) have been addressed.

>> +set_decl_contracts (tree decl, tree contract_attrs)
> I think you want to use 'chainon' here.

>> +build_arg_list (tree fn)
> You can use is_this_parameter here.

Thanks; I knew of chainon but didn't think of it and is_this_parameter
is new to me. Always fun to delete code :)

>> +  /* If we have contracts, check that they're valid in this context.  */
>> +  // FIXME: These aren't entirely correct.
>
> How not?  Can local extern function decls have contract attributes?
>
>> + /* FIXME when we instatiate a template class with guarded
>> +  * members, particularly guarded template members, the 
>> resulting
>> +  * pre/post functions end up being inaccessible because their
>> +  * template info's context is the original uninstantiated 
>> class.
>
> This sounds like a significant functionality gap.  I'm guessing you want
> to reconsider the unshare_template approach.
>
>> +  /* FIXME do we need magic to perfectly forward this so we don't 
>> clobber
>> +RVO/NRVO etc?  */
>
> Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you want.

These points are still being investigated and addressed; including them
for reference.

> More soon.

Please let me know what other issues need work.


Thank you!
Jeff Chapman

Re: [PATCH] PING lra: clear lra_insn_recog_data after simplifying a mem subreg

[Vladimir Makarov via Gcc-patches]

On 2021-01-28 5:40 a.m., Ilya Leoshkevich via Gcc-patches wrote:

Hello,

I would like to ping the following patch:

lra: clear lra_insn_recog_data after simplifying a mem subreg
https://gcc.gnu.org/pipermail/gcc-patches/2021-January/563428.html

Sorry, I missed your original email.  The patch is ok to submit into the 
trunk.


Thank you for addressing the issue.

Re: Patch ping

[Jason Merrill via Gcc-patches]

On 1/25/21 4:43 AM, Jakub Jelinek wrote:


https://gcc.gnu.org/pipermail/gcc-patches/2020-December/560741.html
   dwarf: -gdwarf64 fix for 32-bit targets


OK.

Jason

Re: [PATCH] PING implement pre-c++20 contracts

[Jason Merrill via Gcc-patches]

On 1/4/21 9:58 AM, Jeff Chapman wrote:
Ping. re: 
https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html 



 > OK, I'll start with -alt then, thanks.

Andrew is exactly correct, contracts-jac-alt is still the current
branch we're focusing our upstreaming efforts on.

It's trailing upstream master by a fair bit at this point. I'll get
a merge pushed shortly.


The latest is still on the same branch, which hasn't been updated since 
that last merge:
https://github.com/lock3/gcc/tree/contracts-jac-alt 



Would you prefer me to keep it from trailing upstream too much through 
regular merges, or would it be more beneficial for it to be left alone 
so you have a more stable review target?


Please let me know if there's initial feedback I can start addressing, 
or anything I can do to help the review process along in general.


Why is some of the code in c-family?  From the modules merge there is 
now a cp_handle_option function that you could add the option handling 
to, and I don't see a reason for cxx-contracts.c to be in c-family/ 
rather than cp/.


And then much of the code you add to decl.c could also move to the 
contracts file, and some of the contracts stuff in cp-tree.h could move 
to cxx-contracts.h?



+extern bool cxx23_contract_attribute_p (const_tree);


This name seems optimistic.  :)
Let's call it cxx_contract_attribute_p.


+/* Return TRUE iff ATTR has been parsed by the fornt-end as a c++2a contract


"front"


@@ -566,7 +566,11 @@ decl_attributes (tree *node, tree attributes, int flags,
{
  if (!(flags & (int) ATTR_FLAG_BUILT_IN))
{
- if (ns == NULL_TREE || !cxx11_attr_p)
+ if (cxx23_contract_attribute_p (attr))
+   {
+ ; /* Do not warn about contract "attributes".  */
+   }


I don't want the language-independent code to have to know about this. 
If you want decl_attributes to ignore these attributes, you could give 
these attributes a dummy spec that just returns?



+set_decl_contracts (tree decl, tree contract_attrs)
+{
+  remove_contract_attributes (decl);
+  if (!DECL_ATTRIBUTES (decl))
+{
+  DECL_ATTRIBUTES (decl) = contract_attrs;
+  return;
+}
+  tree last_attr = DECL_ATTRIBUTES (decl);
+  while (TREE_CHAIN (last_attr))
+last_attr = TREE_CHAIN (last_attr);
+  TREE_CHAIN (last_attr) = contract_attrs;


I think you want to use 'chainon' here.


@@ -5498,10 +5863,17 @@ start_decl (const cp_declarator *declarator,
 
   if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)

  /* Aliases are definitions. */
- && !alias)
+ && !alias
+ && (DECL_VIRTUAL_P (decl) || !flag_contracts))
permerror (declarator->id_loc,
   "declaration of %q#D outside of class is not definition",
   decl);
+  else if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
+ /* Aliases are definitions. */
+ && !alias
+ && flag_contract_strict_declarations)
+   warning_at (declarator->id_loc, OPT_fcontract_strict_declarations_,
+   "non-defining declaration of %q#D outside of class", decl);


Let's keep the same message for the two cases.


+void
+finish_function_contracts (tree fndecl, bool is_inline)


This function needs a comment.


+/* cp_tree_defined_p helper -- returns TP if TP is undefined.  */
+
+static tree
+cp_tree_defined_p_r (tree *tp, int *, void *)
+{
+  enum tree_code code = TREE_CODE (*tp);
+  if ((code == FUNCTION_DECL || code == VAR_DECL)
+  && !decl_defined_p (*tp))
+return *tp;
+  /* We never want to accidentally instantiate templates.  */
+  if (code == TEMPLATE_DECL)
+return *tp; /* FIXME? */


In what context are you getting a TEMPLATE_DECL here?  I don't see how 
this would have an effect on instantiations.



+/* Parse a conditional-expression.  */
+/* FIXME: should callers use cp_parser_constant_expression?  */
+
+static cp_expr
+cp_parser_conditional_expression (cp_parser *parser)

...

+  /* FIXME: can we use constant_expression for this?  */
+  cp_expr cond = cp_parser_conditional_expression (parser);


I don't think we want to use cp_parser_constant_expression for 
expressions that are not intended to be constant.



+  bool finishing_guarded_p = true//!processing_template_decl


?


+/* FIXME: Is this going to leak?  */
+comment_str = xstrdup (expr_to_string (cond));


There's no need to strdup here (and free a few lines later); 
build_string_literal copies the bytes.  The return value of 
expr_to_string is in GC memory.



+  /* If we have contracts, check that they're valid in this context.  */
+  // FIXME: These aren't entirely correct.


How not?  Can local extern function decls have contract attributes?


+  if (tree pre = lookup_attribute ("pre", 

Re: [PATCH] PING implement pre-c++20 contracts

[Jason Merrill via Gcc-patches]

On 1/4/21 9:58 AM, Jeff Chapman wrote:
Ping. re: 
https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html 



 > OK, I'll start with -alt then, thanks.

Andrew is exactly correct, contracts-jac-alt is still the current
branch we're focusing our upstreaming efforts on.

It's trailing upstream master by a fair bit at this point. I'll get
a merge pushed shortly.


The latest is still on the same branch, which hasn't been updated since 
that last merge:
https://github.com/lock3/gcc/tree/contracts-jac-alt 



Would you prefer me to keep it from trailing upstream too much through 
regular merges, or would it be more beneficial for it to be left alone 
so you have a more stable review target?


Either way I'm reviewing by diff against the most recent merged trunk 
revision, so it doesn't really matter.


But you probably want to do one merge at least, to make sure that 
modules and contracts coexist well.


Jason

Re: [PATCH] [PING^2] Asan changes for RISC-V.

[Jim Wilson]

Original message here
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557406.html

This has non-RISC-V changes, so I need a global reviewer to look at it.

Jim

On Wed, Nov 4, 2020 at 12:10 PM Jim Wilson  wrote:

>
>
> On Wed, Oct 28, 2020 at 4:59 PM Jim Wilson  wrote:
>
>> We have only riscv64 asan support, there is no riscv32 support as yet.
>> So I
>> need to be able to conditionally enable asan support for the riscv
>> target.  I
>> implemented this by returning zero from the asan_shadow_offset function.
>> This
>> requires a change to toplev.c and docs in target.def.
>>
>> The asan support works on a 5.5 kernel, but does not work on a 4.15
>> kernel.
>> The problem is that the asan high memory region is a small wedge below
>> 0x40.  The new kernel puts shared libraries at 0x3f and
>> going
>> down which works.  But the old kernel puts shared libraries at
>> 0x20
>> and going up which does not work, as it isn't in any recognized memory
>> region.  This might be fixable with more asan work, but we don't really
>> need
>> support for old kernel versions.
>>
>> The asan port is curious in that it uses 1<<29 for the shadow offset, but
>> all
>> other 64-bit targets use a number larger than 1<<32.  But what we have is
>> working OK for now.
>>
>> I did a make check RUNTESTFLAGS="asan.exp" on Fedora rawhide image
>> running on
>> qemu and the results look reasonable.
>>
>> === gcc Summary ===
>>
>> # of expected passes1905
>> # of unexpected failures11
>> # of unsupported tests  224
>>
>> === g++ Summary ===
>>
>> # of expected passes2002
>> # of unexpected failures6
>> # of unresolved testcases   1
>> # of unsupported tests  175
>>
>> OK?
>>
>> Jim
>>
>> 2020-10-28  Jim Wilson  
>>
>> gcc/
>> * config/riscv/riscv.c (riscv_asan_shadow_offset): New.
>> (TARGET_ASAN_SHADOW_OFFSET): New.
>> * doc/tm.texi: Regenerated.
>> * target.def (asan_shadow_offset); Mention that it can return
>> zero.
>> * toplev.c (process_options): Check for and handle zero return
>> from
>> targetm.asan_shadow_offset call.
>>
>> Co-Authored-By: cooper.joshua 
>> ---
>>  gcc/config/riscv/riscv.c | 16 
>>  gcc/doc/tm.texi  |  3 ++-
>>  gcc/target.def   |  3 ++-
>>  gcc/toplev.c |  3 ++-
>>  4 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
>> index 989a9f15250..6909e200de1 100644
>> --- a/gcc/config/riscv/riscv.c
>> +++ b/gcc/config/riscv/riscv.c
>> @@ -5299,6 +5299,19 @@ riscv_gpr_save_operation_p (rtx op)
>>return true;
>>  }
>>
>> +/* Implement TARGET_ASAN_SHADOW_OFFSET.  */
>> +
>> +static unsigned HOST_WIDE_INT
>> +riscv_asan_shadow_offset (void)
>> +{
>> +  /* We only have libsanitizer support for RV64 at present.
>> +
>> + This number must match kRiscv*_ShadowOffset* in the file
>> + libsanitizer/asan/asan_mapping.h which is currently 1<<29 for rv64,
>> + even though 1<<36 makes more sense.  */
>> +  return TARGET_64BIT ? (HOST_WIDE_INT_1 << 29) : 0;
>> +}
>> +
>>  /* Initialize the GCC target structure.  */
>>  #undef TARGET_ASM_ALIGNED_HI_OP
>>  #define TARGET_ASM_ALIGNED_HI_OP "\t.half\t"
>> @@ -5482,6 +5495,9 @@ riscv_gpr_save_operation_p (rtx op)
>>  #undef TARGET_NEW_ADDRESS_PROFITABLE_P
>>  #define TARGET_NEW_ADDRESS_PROFITABLE_P riscv_new_address_profitable_p
>>
>> +#undef TARGET_ASAN_SHADOW_OFFSET
>> +#define TARGET_ASAN_SHADOW_OFFSET riscv_asan_shadow_offset
>> +
>>  struct gcc_target targetm = TARGET_INITIALIZER;
>>
>>  #include "gt-riscv.h"
>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>> index 24c37f655c8..39c596b647a 100644
>> --- a/gcc/doc/tm.texi
>> +++ b/gcc/doc/tm.texi
>> @@ -12078,7 +12078,8 @@ is zero, which disables this optimization.
>>  @deftypefn {Target Hook} {unsigned HOST_WIDE_INT}
>> TARGET_ASAN_SHADOW_OFFSET (void)
>>  Return the offset bitwise ored into shifted address to get corresponding
>>  Address Sanitizer shadow memory address.  NULL if Address Sanitizer is
>> not
>> -supported by the target.
>> +supported by the target.  May return 0 if Address Sanitizer is not
>> supported
>> +by a subtarget.
>>  @end deftypefn
>>
>>  @deftypefn {Target Hook} {unsigned HOST_WIDE_INT} TARGET_MEMMODEL_CHECK
>> (unsigned HOST_WIDE_INT @var{val})
>> diff --git a/gcc/target.def b/gcc/target.def
>> index ed2da154e30..268b56b6ebd 100644
>> --- a/gcc/target.def
>> +++ b/gcc/target.def
>> @@ -4452,7 +4452,8 @@ DEFHOOK
>>  (asan_shadow_offset,
>>   "Return the offset bitwise ored into shifted address to get
>> corresponding\n\
>>  Address Sanitizer shadow memory address.  NULL if Address Sanitizer is
>> not\n\
>> -supported by the target.",
>> +supported by the target.  May return 0 if Address Sanitizer is not
>> supported\n\
>> +by a subtarget.",
>>   unsigned HOST_WIDE_INT, (void),
>>   NULL)
>>
>> diff --git 

Re: [PATCH] [PING] Asan changes for RISC-V.

[Kito Cheng via Gcc-patches]

LGTM.

Verified with Fedora rawhide image running on qemu, kernel version is 5.5.0,
I got slightly different gcc testsuite result, but after reviewing all
failed cases,
it should not be a blocker for this patch.

It  seems environment issue, some minor issues like stack
unwinding or library search path.
e.g
- ld complain cannot find -latomic
- Stack unwinding result not show main in asan report.

=== gcc Summary ===

# of expected passes2672
# of unexpected failures25
# of unresolved testcases   14
# of unsupported tests  224
=== g++ Summary ===

# of expected passes1967
# of unexpected failures20
# of unresolved testcases   15
# of unsupported tests  175




On Thu, Nov 5, 2020 at 4:11 AM Jim Wilson  wrote:
>
> On Wed, Oct 28, 2020 at 4:59 PM Jim Wilson  wrote:
>
> > We have only riscv64 asan support, there is no riscv32 support as yet.  So
> > I
> > need to be able to conditionally enable asan support for the riscv
> > target.  I
> > implemented this by returning zero from the asan_shadow_offset function.
> > This
> > requires a change to toplev.c and docs in target.def.
> >
> > The asan support works on a 5.5 kernel, but does not work on a 4.15 kernel.
> > The problem is that the asan high memory region is a small wedge below
> > 0x40.  The new kernel puts shared libraries at 0x3f and
> > going
> > down which works.  But the old kernel puts shared libraries at 0x20
> > and going up which does not work, as it isn't in any recognized memory
> > region.  This might be fixable with more asan work, but we don't really
> > need
> > support for old kernel versions.
> >
> > The asan port is curious in that it uses 1<<29 for the shadow offset, but
> > all
> > other 64-bit targets use a number larger than 1<<32.  But what we have is
> > working OK for now.
> >
> > I did a make check RUNTESTFLAGS="asan.exp" on Fedora rawhide image running
> > on
> > qemu and the results look reasonable.
> >
> > === gcc Summary ===
> >
> > # of expected passes1905
> > # of unexpected failures11
> > # of unsupported tests  224
> >
> > === g++ Summary ===
> >
> > # of expected passes2002
> > # of unexpected failures6
> > # of unresolved testcases   1
> > # of unsupported tests  175
> >
> > OK?
> >
> > Jim
> >
> > 2020-10-28  Jim Wilson  
> >
> > gcc/
> > * config/riscv/riscv.c (riscv_asan_shadow_offset): New.
> > (TARGET_ASAN_SHADOW_OFFSET): New.
> > * doc/tm.texi: Regenerated.
> > * target.def (asan_shadow_offset); Mention that it can return zero.
> > * toplev.c (process_options): Check for and handle zero return from
> > targetm.asan_shadow_offset call.
> >
> > Co-Authored-By: cooper.joshua 
> > ---
> >  gcc/config/riscv/riscv.c | 16 
> >  gcc/doc/tm.texi  |  3 ++-
> >  gcc/target.def   |  3 ++-
> >  gcc/toplev.c |  3 ++-
> >  4 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
> > index 989a9f15250..6909e200de1 100644
> > --- a/gcc/config/riscv/riscv.c
> > +++ b/gcc/config/riscv/riscv.c
> > @@ -5299,6 +5299,19 @@ riscv_gpr_save_operation_p (rtx op)
> >return true;
> >  }
> >
> > +/* Implement TARGET_ASAN_SHADOW_OFFSET.  */
> > +
> > +static unsigned HOST_WIDE_INT
> > +riscv_asan_shadow_offset (void)
> > +{
> > +  /* We only have libsanitizer support for RV64 at present.
> > +
> > + This number must match kRiscv*_ShadowOffset* in the file
> > + libsanitizer/asan/asan_mapping.h which is currently 1<<29 for rv64,
> > + even though 1<<36 makes more sense.  */
> > +  return TARGET_64BIT ? (HOST_WIDE_INT_1 << 29) : 0;
> > +}
> > +
> >  /* Initialize the GCC target structure.  */
> >  #undef TARGET_ASM_ALIGNED_HI_OP
> >  #define TARGET_ASM_ALIGNED_HI_OP "\t.half\t"
> > @@ -5482,6 +5495,9 @@ riscv_gpr_save_operation_p (rtx op)
> >  #undef TARGET_NEW_ADDRESS_PROFITABLE_P
> >  #define TARGET_NEW_ADDRESS_PROFITABLE_P riscv_new_address_profitable_p
> >
> > +#undef TARGET_ASAN_SHADOW_OFFSET
> > +#define TARGET_ASAN_SHADOW_OFFSET riscv_asan_shadow_offset
> > +
> >  struct gcc_target targetm = TARGET_INITIALIZER;
> >
> >  #include "gt-riscv.h"
> > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> > index 24c37f655c8..39c596b647a 100644
> > --- a/gcc/doc/tm.texi
> > +++ b/gcc/doc/tm.texi
> > @@ -12078,7 +12078,8 @@ is zero, which disables this optimization.
> >  @deftypefn {Target Hook} {unsigned HOST_WIDE_INT}
> > TARGET_ASAN_SHADOW_OFFSET (void)
> >  Return the offset bitwise ored into shifted address to get corresponding
> >  Address Sanitizer shadow memory address.  NULL if Address Sanitizer is not
> > -supported by the target.
> > +supported by the target.  May return 0 if Address Sanitizer is not
> > supported
> > +by a subtarget.
> >  @end