[Bug c++/112296] New: __builtin_constant_p doesn't propagate through member functions

[Bug c++/112296] New: __builtin_constant_p doesn't propagate through member functions

[barry.revzin at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

            Bug ID: 112296
           Summary: __builtin_constant_p doesn't propagate through member
                    functions
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: barry.revzin at gmail dot com
  Target Milestone: ---

Here's a short example:

using size_t = decltype(sizeof(0));

struct Span {
    int const* ptr;
    size_t len;

    inline constexpr auto size() const noexcept -> size_t { return len; }
};

inline int direct(Span span) {
    return __builtin_constant_p(span.size());
}

inline int indirect(Span span) {
    size_t s = span.size();
    return __builtin_constant_p(s);
}

int call_direct(int const* p) {
    return direct({.ptr=p, .len=8});
}

int call_indirect(int const* p) {
    return indirect({.ptr=p, .len=8});
}



The functions direct() and indirect() do the same thing - try to see if span's
size is constant, with direct checking size() directly and indirect first
caching it to a local variable and checking that variable. In both cases, the
size is constant - but on -O3 call_direct() returns 0 while call_indirect()
returns 1. That is, __builtin_constant_p tells me the size is constant - but
only if I put it into a variable first. 

Checking __builtin_constant_p(span.len) also returns 1, but in the real code
the member variable is private and my only access to it is via the size()
function.

[Bug c++/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
__builtin_constant_p is limited by inlining, maybe you want to use
if constexpr for this or the standard library facility I can't remember
right now.

[Bug c++/112296] __builtin_constant_p doesn't propagate through member functions

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think you mean std::is_constant_evaluated, but that doesn't check values, it
checks if the function is being evaluated as constexpr. And if-constexpr can't
be used here either, that works like templates, so only on static types, not
runtime values.

[Bug c++/112296] __builtin_constant_p doesn't propagate through member functions

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Barry Revzin from comment #0)
> inline int direct(Span span) {
>     return __builtin_constant_p(span.size());
> }
> 
> inline int indirect(Span span) {
>     size_t s = span.size();
>     return __builtin_constant_p(s);
> }

If the problem is that bcp is limited by inlining, you'd think that the first
one would be more likely to work. It does less.

Clearly inlining can see through the member function, because in 'indirect' it
can propagate its return value through 's' and to the bcp call.

But in 'direct' the return value is somehow not constant when used directly. It
is only constant if propagated through a local variable. That seems like an
unnecessary limitation.

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2023-10-31
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
The issue is that we're inlining "optimized" bodies but somehow "optimize"
direct() already in the frontend to

;; Function int direct(Span) (null)
;; enabled by -tree-original


<<cleanup_point return <retval> = 0>>;


that's because we have builtins.cc:fold_builtin_constant_p

  /* If this expression has side effects, show we don't know it to be a
     constant.  Likewise if it's a pointer or aggregate type since in
     those case we only want literals, since those are only optimized
     when generating RTL, not later.
     And finally, if we are compiling an initializer, not code, we
     need to return a definite result now; there's not going to be any
     more optimization done.  */
  if (TREE_SIDE_EFFECTS (arg)
      || AGGREGATE_TYPE_P (TREE_TYPE (arg))
      || POINTER_TYPE_P (TREE_TYPE (arg))
      || cfun == 0
      || folding_initializer
      || force_folding_builtin_constant_p)
    return integer_zero_node;

and the CALL_EXPR we have as argument has side-effects.

That all looks like from prehistoric times, but I'm only going to remove
the TREE_SIDE_EFFECTS check which makes the direct and indirect cases
produce the same result.

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |documentation
                 CC|                            |jsm28 at gcc dot gnu.org,
                   |                            |rth at gcc dot gnu.org

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
+FAIL: gcc.c-torture/execute/bcp-1.c   -O1  (test for excess errors)
+UNRESOLVED: gcc.c-torture/execute/bcp-1.c   -O1  compilation failed to produce 
executable

that's because

bcp-1.c:(.text+0x6c): undefined reference to `func'^M
collect2: error: ld returned 1 exit status^M

the testcase can be reduced to

int func(void);
int bad7(void) { return __builtin_constant_p(func()); } 

and we want the side-effect to disappear!?  There's more fun in this testcase,
like verifying that

int bad1(void) { return __builtin_constant_p(global++); }

returns 0 (but not verifying whether 'global' is altered or not).

I don't see it documented that __builtin_constant_p discards side-effects on
operands - that would in fact make it behave like 'constexpr'?!

commit 54303b5cb3076f7bae9102c08b59d57b1a9f5104
Author: Richard Henderson <rth@cygnus.com>
Date:   Tue Jul 7 06:41:17 1998 -0700

    * gcc.c-torture/execute/bcp-1.c: New test.

So this also becomes a documentation thing and I doubt we can start to
retain side-effects we've thrown away for decades now.  But then
relying on _folding_ to implement what becomes a language extension this
way is definitely error-prone and fragile.

I think we should also definitely diagnose this somehow (which would also
mean moving discarding of side-effects to the frontend(s)).

Joseph, Richard, do you have anything to add or remember discussions about
this semantic detail of __builtin_constant_p?

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
For the C++ frontend I wonder why the span.size () call is marked as
TREE_SIDE_EFFECTS for a inline constexpr auto size() const noexcept
function.  Possibly those declaration specifiers are not enough to
guarantee the lack of side-effects?

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #7 from Richard Henderson <rth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #5)
> int bad1(void) { return __builtin_constant_p(global++); }
...
> Joseph, Richard, do you have anything to add or remember discussions about
> this semantic detail of __builtin_constant_p?

Since it has been 25 years, I don't recall any specific discussions.

The intended use-case at the time was more like

#define macro(x) \
  (__builtin_constant_p(x) \
   ? inline_expression(x)  \
   : out_of_line_function(x))

So I would have expected side effects to have been ignored for
the builtin and expanded via one of the two arms.

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Henderson from comment #7)
> (In reply to Richard Biener from comment #5)
> > int bad1(void) { return __builtin_constant_p(global++); }
> ...
> > Joseph, Richard, do you have anything to add or remember discussions about
> > this semantic detail of __builtin_constant_p?
> 
> Since it has been 25 years, I don't recall any specific discussions.
> 
> The intended use-case at the time was more like
> 
> #define macro(x) \
>   (__builtin_constant_p(x) \
>    ? inline_expression(x)  \
>    : out_of_line_function(x))
> 
> So I would have expected side effects to have been ignored for
> the builtin and expanded via one of the two arms.

Thanks.  So yes,

macro(x++);

incrementing x twice would have been odd - but that's the usual bug
in this kind of macro definition.  Fixing it by throwing away
side-effects (and always going the out_of_line_function (x) path!)
for the __builtin_constant_p argument is an odd choice.

As said, I wondered if that was deliberate or simply an omission of

   return omit_one_operand (integer_type_node, integer_zero_node, arg);

...

The execute.exp testcase suggests the intention but the testcases
verification is somewhat incomplete (it lacks verifying the side-effects
are gone).

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #9 from Richard Henderson <rth at gcc dot gnu.org> ---
> Thanks.  So yes,
> 
> macro(x++);
> 
> incrementing x twice would have been odd - but that's the usual bug
> in this kind of macro definition.  Fixing it by throwing away
> side-effects (and always going the out_of_line_function (x) path!)
> for the __builtin_constant_p argument is an odd choice.

In the beginning __builtin_constant_p was resolved immediately,
so formulating this as

#define macro(x) \
  ({ __typeof(x) _x = (x); \
     __builtin_constant_p(_x) })

would always return false, defeating the purpose.

> The execute.exp testcase suggests the intention but the testcases
> verification is somewhat incomplete (it lacks verifying the side-effects
> are gone).

That's probably my omission.  ;-)

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
Some history shows r0-31250-gb0b3afb2f34492 splitting this to the folding
code from RTL expansion of __builtin_constant_p where it previously was.
Even before r0-17364-gcff48d8f76463f we were simply doing

@@ -8566,10 +8579,34 @@ expand_builtin (exp, target, subtarget, mode, ignore)
          tree arg = TREE_VALUE (arglist);

          STRIP_NOPS (arg);
-         return (TREE_CODE_CLASS (TREE_CODE (arg)) == 'c'
-                 || (TREE_CODE (arg) == ADDR_EXPR
-                     && TREE_CODE (TREE_OPERAND (arg, 0)) == STRING_CST)
-                 ? const1_rtx : const0_rtx);

and that dates back to r0-384-gbbf6f052d786c2, "Initial revision" of expr.c
from Kenner :/

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|rguenth at gcc dot gnu.org         |unassigned at gcc dot gnu.org
             Status|ASSIGNED                    |NEW
                 CC|                            |jason at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
So as for the original description of this bug - it's a feature then (unless it
gets clarified in another way).  Sorry.  I agree it's highly unintuitive and I
would hope we can eventually add diagnostics for __builtin_constant_p with
arguments that have side-effects.

At least the documentation should be amended.  Waiting for C/C++ frontend
maintainers to give an ACK, then I'll propose some wording for the
documentation.

Btw, clang also implements the weird semantics.

int g;
int bar () { if (!g) abort (); return 1; }

int main()
{
  return __builtin_constant_p (bar());
}

returns 0 and doesn't abort().

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #12 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I agree that the side effects of an argument to __builtin_constant_p must 
be discarded, for the original macro use case to work properly.

There are various constructs with __builtin_* names that, although they 
look like function calls, in fact have syntactic or semantic differences 
from what can be done with a normal function call.  In the cases of 
syntactic differences, they are actually keywords and handled specially in 
the parsers.  That's probably not relevant here, because the issue is 
semantics of the call (argument not evaluated) rather than the syntax, but 
it does illustrate how it's reasonable to have special handling for some 
__builtin_* construct when needed for its semantics.

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
I've posted a documentation patch.

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:442715708911ed6cc6f3785e3996a62f5ee7f21f

commit r14-5143-g442715708911ed6cc6f3785e3996a62f5ee7f21f
Author: Richard Biener <rguenther@suse.de>
Date:   Fri Nov 3 08:11:05 2023 +0100

    middle-end/112296 - __builtin_constant_p and side-effects

    The following tries to clarify the __builtin_constant_p documentation,
    stating that the argument expression is not evaluated and side-effects
    are discarded.  I'm struggling to find the correct terms matching
    what the C language standard would call things so I'd appreciate
    some help here.

    OK for trunk?

    Shall we diagnose arguments with side-effects?  It seems to me
    such use is usually unintended?  I think rather than dropping
    side-effects as a side-effect of folding the frontend should
    discard them at parsing time instead, no?

    Thanks,
    Richard.

            PR middle-end/112296
            * doc/extend.texi (__builtin_constant_p): Clarify that
            side-effects are discarded.

[Bug tree-optimization/112296] __builtin_constant_p doesn't propagate through member functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112296

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
           Keywords|documentation               |
                 CC|                            |jakub at gcc dot gnu.org
           Assignee|rguenth at gcc dot gnu.org         |unassigned at gcc dot gnu.org

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
I clarified the documentation.  While in the particular case 'span.size ()'
doesn't have side-effects (well, I'm not 100% sure) in general function calls
are difficult to handle "delayed" while still allowing to discard side-effects.

Maybe the infrastructure we put in place for [[assume]] could help here by
treating __builtin_constant_p (expr) as

 [["assume" (guard)]]
 {
   tem = expr;
   guard = __builtin_constant_p (tem);
 }
 guard;

and lower it to

 .ASSUME (&artificial_fn, args...);

but now with a return value that would become zero at the point we elide
.ASSUME unless we end up with a [1,1] range for the result?


That said, it's best to avoid relying on the side-effect removal, that is,
avoid having arguments with possible side-effects in __builtin_constant_p.