Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libgccjit: Fix get_size of size_t #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

antoyo
Copy link
Owner

@antoyo antoyo commented Oct 16, 2024

gcc/jit/ChangeLog:
	PR jit/112910
	* jit-recording.cc (recording::memento_of_get_type::get_size):
	Correctly compute the size of size_t.
@antoyo antoyo added waiting for review Waiting for review from upstream libgccjit maintainer waiting for info and removed waiting for review Waiting for review from upstream libgccjit maintainer labels Oct 17, 2024
@antoyo
Copy link
Owner Author

antoyo commented Nov 19, 2024

@davidmalcolm:

There's one issue with this patch: like every other feature that
checks
for target-specific stuff, it requires a compilation before
actually
fetching the size of the type.
Which means that getting the size before a compilation might be
wrong
(and I actually believe is wrong on x86-64).

I was wondering if we should always implicitely do the first
compilation to gather the correct info: this would fix this issue
and
all the others that we have due to that.
I'm not sure what would be the performance implication.

Maybe introduce a new class target_info which contains all the
information we might want to find via a compilation, and have the
top-
level recording::context have a pointer to it, which starts as
nullptr,
but can be populated on-demand the first time something needs it?

That would mean that we'll need to populate it for every top-level
context, right? Would the idea be that we should then use child
contexts to have the proper information filled?
If so, how is this different than just compiling two contexts like
what
I currently do?
This would also mean that we'll do an implicit compilation whenever
we
use an API that needs this info, right? Wouldn't that be unexpected?

I was thinking a compilation with an empty playback::context to lazily
capture the target data.

I'm still not sure I understand what you mean.
Do you mean having a global context that we can compile to then fetch the size of the types?
If not, could you please provide an example with some code?

I'm wondering if we could have something that would also work for custom types like structs.
I'm also not sure what would happen if options that change the size of types (like -m32) are provided by the user.

Is the way libgccjit currently work (with 2 phases: recording and playback) this way because gcc is not thread-safe?
If we could directly create GENERIC trees, we could get the size from those, but it seems like this would not be possible.

antoyo pushed a commit that referenced this pull request Nov 20, 2024
gcc.dg/torture/pr112305.c contains an inner loop that executes
0x8000_0014 times and an outer loop that executes 5 times, giving about
10 billion total executions of the inner loop body.  At -O2 and above we
are able to remove the inner loop, but at -O1 we keep a no-op loop:

        dls     lr, r3
.L3:
        subs    r3, r3, #1
        le      lr, .L3

and at -O0 we of course don't optimise.

This can lead to long execution times on simulators, possibly
triggering a timeout.

gcc/testsuite
	* gcc.dg/torture/pr112305.c: Skip at -O0 and -O1 for simulators.
antoyo pushed a commit that referenced this pull request Nov 20, 2024
We currently crash upon the following invalid code (notice the "void
void**" parameter)

=== cut here ===
using size_t = decltype(sizeof(int));
void *operator new(size_t, void void **p) noexcept { return p; }
int x;
void f() {
    int y;
    new (&y) int(x);
}
=== cut here ===

The problem is that in this case, we end up with a NULL_TREE parameter
list for the new operator because of the error, and (1) coerce_new_type
wrongly complains about the first parameter type not being size_t,
(2) std_placement_new_fn_p blindly accesses the parameter list, hence a
crash.

This patch does NOT address #1 since we can't easily distinguish between
a new operator declaration without parameters from one with erroneous
parameters (and it's not worth the risk to refactor and break things for
an error recovery issue) hence a dg-bogus in new52.C, but it does
address #2 and the ICE by simply checking the first parameter against
NULL_TREE.

It also adds a new testcase checking that we complain about new
operators with no or invalid first parameters, since we did not have
any.

	PR c++/117101

gcc/cp/ChangeLog:

	* init.cc (std_placement_new_fn_p): Check first_arg against
	NULL_TREE.

gcc/testsuite/ChangeLog:

	* g++.dg/init/new52.C: New test.
	* g++.dg/init/new53.C: New test.
antoyo pushed a commit that referenced this pull request Nov 20, 2024
Update test case for armv8.1-m.main that supports conditional
arithmetic.

armv7-m:
        push    {r4, lr}
        ldr     r4, .L6
        ldr     r4, [r4]
        lsls    r4, r4, gcc-mirror#29
        it      mi
        addmi   r2, r2, #1
        bl      bar
        movs    r0, #0
        pop     {r4, pc}

armv8.1-m.main:
        push    {r3, r4, r5, lr}
        ldr     r4, .L5
        ldr     r5, [r4]
        tst     r5, #4
        csinc   r2, r2, r2, eq
        bl      bar
        movs    r0, #0
        pop     {r3, r4, r5, pc}

gcc/testsuite/ChangeLog:

	* gcc.target/arm/epilog-1.c: Use check-function-bodies.

Signed-off-by: Torbjörn SVENSSON <[email protected]>
antoyo pushed a commit that referenced this pull request Nov 20, 2024
The second source register of insn "*extzvsi-1bit_addsubx" cannot be the
same as the destination register, because that register will be overwritten
with an intermediate value after insn splitting.

     /* example #1 */
     int test1(int b, int a) {
       return ((a & 1024) ? 4 : 0) + b;
     }

     ;; result #1 (incorrect)
     test1:
     	extui	a2, a3, 10, 1	;; overwrites A2 before used
     	addx4	a2, a2, a2
     	ret.n

This patch fixes that.

     ;; result #1 (correct)
     test1:
     	extui	a3, a3, 10, 1	;; uses A3 and then overwrites
     	addx4	a2, a3, a2
     	ret.n

However, it should be noted that the first source register can be the same
as the destination without any problems.

     /* example #2 */
     int test2(int a, int b) {
       return ((a & 1024) ? 4 : 0) + b;
     }

     ;; result (correct)
     test2:
     	extui	a2, a2, 10, 1	;; uses A2 and then overwrites
     	addx4	a2, a2, a3
     	ret.n

gcc/ChangeLog:

	* config/xtensa/xtensa.md (*extzvsi-1bit_addsubx):
	Add '&' to the destination register constraint to indicate that
	it is 'earlyclobber', append '0' to the first source register
	constraint to indicate that it can be the same as the destination
	register, and change the split condition from 1 to reload_completed
	so that the insn will be split only after RA in order to obtain
	allocated registers that satisfy the above constraints.
antoyo pushed a commit that referenced this pull request Dec 5, 2024
vec.h has this method:

  template<typename T, typename A>
  inline T *
  vec_safe_push (vec<T, A, vl_embed> *&v, const T &obj CXX_MEM_STAT_INFO)

where v is a reference to a pointer to vec.  This matches the regex for
VecPrinter, so gdbhooks.py attempts to print it but chokes on the reference.
I see the following:

  #1  0x0000000002b84b7b in vec_safe_push<edge_def*, va_gc> (v=Traceback (most
  recent call last):
    File "$SRC/gcc/gcc/gdbhooks.py", line 486, in to_string
      return '0x%x' % intptr(self.gdbval)
    File "$SRC/gcc/gcc/gdbhooks.py", line 168, in intptr
      return long(gdbval) if sys.version_info.major == 2 else int(gdbval)
  gdb.error: Cannot convert value to long.

This patch makes VecPrinter handle such references by stripping them
(dereferencing) at the top of the relevant functions.

gcc/ChangeLog:

	* gdbhooks.py (strip_ref): New. Use it ...
	(VecPrinter.to_string): ... here,
	(VecPrinter.children): ... and here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant