From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 809F83858409 for ; Tue, 18 Jan 2022 23:06:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 809F83858409 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-641-EXwFqu_8NtiMI03jP7ov1Q-1; Tue, 18 Jan 2022 18:06:18 -0500 X-MC-Unique: EXwFqu_8NtiMI03jP7ov1Q-1 Received: by mail-qk1-f199.google.com with SMTP id z191-20020a3765c8000000b00478d7915c95so517997qkb.9 for ; Tue, 18 Jan 2022 15:06:18 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=m2XuBx1QZNr2KsUD18sEzMoDG6pOHe0nVr/QmXYMSXk=; b=MkR4Lmq2jt1GhfYUMRtGfpGmf3kXFBOHEYOkTggiHp7GoZmXFe6C256XiEOOYWjhTh hQZ275gaMQUyznHGoAv/mQ240P3WQCfb75WlxJ5HYV0rm565E8fFRG4EByhqyAb/tmCq clIvv3DZunhYbipX983x4X0W7MWcqCP36wcEYwZ5gt1o5Al1Grqj3XYAz9RMGgVT0UGY U//ajiFEk+r6bGpmyBRDc8lDwxt69lLRszvXRhtJ35fj5r/0EeN4O9jv2J6zF4GsTGkk ASkO9l8SWs+AcmjyrGkVzArYPZj5PbfpXncrAI2xng5diNZuYnKT2sCndmEZZTKyhqs3 M1gg== X-Gm-Message-State: AOAM5318w2Dd2hJKqpccbbxTLomGo/tfR2no9YrJ43dt1Uhn4kbxHzcC qF3mirHy9EuJIs5tLFpAX5PbhR6SiVefxUDRMAow/S7pg4Vxly2SWOrrSRilsgf0iHLXAkZtgYy uIeSSgO0= X-Received: by 2002:a05:620a:15f6:: with SMTP id p22mr14973036qkm.500.1642547177505; Tue, 18 Jan 2022 15:06:17 -0800 (PST) X-Google-Smtp-Source: ABdhPJxHikOH2LjwL7tBhzTcEzl90Uh38kazL6NsOGjJjtwsK47RcGl2j5pw/Q+5xYaJ8qC4qZ2E1w== X-Received: by 2002:a05:620a:15f6:: with SMTP id p22mr14973012qkm.500.1642547177029; Tue, 18 Jan 2022 15:06:17 -0800 (PST) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id i7sm12159790qkn.0.2022.01.18.15.06.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Jan 2022 15:06:16 -0800 (PST) Message-ID: Subject: Re: [PATCH] libgccjit: Add support for bitcasts [PR104071] From: David Malcolm To: Antoni Boucher , gcc-patches@gcc.gnu.org, jit@gcc.gnu.org Date: Tue, 18 Jan 2022 18:06:15 -0500 In-Reply-To: <19fe391c6c77e0e38acdcaf7e864651849913d46.camel@zoho.com> References: <13bdd05cfd7006a332ec785ab371356f0354db4d.camel@zoho.com> <19fe391c6c77e0e38acdcaf7e864651849913d46.camel@zoho.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jan 2022 23:06:23 -0000 On Mon, 2022-01-17 at 19:30 -0500, Antoni Boucher via Gcc-patches wrote: > I was missing the define, so I added it. > Here's the new patch with it. Thanks for the patch. > Le lundi 17 janvier 2022 à 17:18 -0500, Antoni Boucher via Jit a > écrit : > > Hi. > > This patch add support for bitcasts in libgccjit. > > > > It passes the JIT tests, but since I added a function in tree.c, I > > wonder if I should run the whole testsuite. We're in stage 4 for GCC 12 now, so we need to be especially careful and conservative about every change. A strict reading on the rules is that we shouldn't be adding new features - but if they're confined to libgccjit we may be able to get release manager approval. > > > > David, you can now disregard my question in my email about 128-bit > > integers regarding my issue with initialize_sizetypes being called > > multiple times because this patch fix this issue. > > I turns out there was a cache of types that needed to be cleared > > when > > you initialize the JIT. > > > > The check for sizes is pending, because it requires the updates to > > get_size I made in my patch for 128-bit integers. Sorry, I seem to have mislaid that patch; do you have the "Subject" line handy? Do you have a list of the patches I need to review? As for this patch, overall I like it, but there are various nits... > > > > Thanks for the review! > 2022-01-17 Antoni Boucher > > gcc/jit/ > PR target/104071 Should be "jit" rather than "target". Various source files are now .cc rather than .c after yesterday's big renaming. > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_20): New ABI tag. > * docs/topics/expressions.rst: Add documentation for the > function gcc_jit_context_new_bitcast. > * dummy-frontend.c: clear the cache of non-standard integer > types to avoid having issues with some optimizations of > bitcast where the SSA_NAME will have a size of a cached > integer type that should have been invalidated, causing a > comparison of integer constant to fail. > * jit-playback.c: New function (new_bitcast). > * jit-playback.h: New function (new_bitcast). > * jit-recording.c: New functions (new_bitcast, > bitcast::replay_into, bitcast::visit_children, > bitcast::make_debug_string, bitcast::write_reproducer). > * jit-recording.h: New calss (bitcast) and new function > (new_bitcast, bitcast::replay_into, bitcast::visit_children, > bitcast::make_debug_string, bitcast::write_reproducer, > bitcast::get_precedence). > * libgccjit.c: New function (gcc_jit_context_new_bitcast) > * libgccjit.h: New function (gcc_jit_context_new_bitcast) > * libgccjit.map (LIBGCCJIT_ABI_20): New ABI tag. > > gcc/testsuite/ > PR target/104071 > * jit.dg/all-non-failing-tests.h: Add new test-bitcast. > * jit.dg/test-bitcast.c: New test. > > gcc/ > PR target/104071 > * tree.c: New function (clear_nonstandard_integer_type_cache). > * tree.h: New function (clear_nonstandard_integer_type_cache). > --- > gcc/jit/docs/topics/compatibility.rst | 9 +++ > gcc/jit/docs/topics/expressions.rst | 17 +++++ > gcc/jit/dummy-frontend.c | 2 + > gcc/jit/jit-playback.c | 13 ++++ > gcc/jit/jit-playback.h | 5 ++ > gcc/jit/jit-recording.c | 66 ++++++++++++++++++++ > gcc/jit/jit-recording.h | 32 ++++++++++ > gcc/jit/libgccjit.c | 28 +++++++++ > gcc/jit/libgccjit.h | 15 +++++ > gcc/jit/libgccjit.map | 6 ++ > gcc/testsuite/jit.dg/all-non-failing-tests.h | 10 +++ > gcc/testsuite/jit.dg/test-bitcast.c | 60 ++++++++++++++++++ > gcc/tree.c | 8 +++ > gcc/tree.h | 1 + > 14 files changed, 272 insertions(+) > create mode 100644 gcc/testsuite/jit.dg/test-bitcast.c > > diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst > index 16cebe31a10..b5a6b704dda 100644 > --- a/gcc/jit/docs/topics/compatibility.rst > +++ b/gcc/jit/docs/topics/compatibility.rst > @@ -302,3 +302,12 @@ thread-local storage model of a variable: > section of a variable: > > * :func:`gcc_jit_lvalue_set_link_section` > + > +.. _LIBGCCJIT_ABI_20: > + > +``LIBGCCJIT_ABI_20`` > +----------------------- > +``LIBGCCJIT_ABI_20`` covers the addition of an API entrypoint to bitcast a > +value from one type to another: > + > + * :func:`gcc_jit_context_new_bitcast` > diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst > index 791a20398ca..1328a53f70f 100644 > --- a/gcc/jit/docs/topics/expressions.rst > +++ b/gcc/jit/docs/topics/expressions.rst > @@ -649,6 +649,23 @@ Type-coercion > * int <-> bool > * P* <-> Q*, for pointer types P and Q > > +.. function:: gcc_jit_rvalue *\ > + gcc_jit_context_new_bitcast (gcc_jit_context *ctxt,\ > + gcc_jit_location *loc,\ > + gcc_jit_rvalue *rvalue,\ > + gcc_jit_type *type) > + > + Given an rvalue of T, bitcast it to another type. I think it's worth defining what "bitcast" means in the docs here; presumably you mean something like "generating a new rvalue by interpreting the bits of the input rvalue according to the layout of type" or somesuch. > + > + The type of rvalue must be the same size as the size of ``type``. > + > + This entrypoint was added in :ref:`LIBGCCJIT_ABI_20`; you can test for > + its presence using > + > + .. code-block:: c > + > + #ifdef LIBGCCJIT_HAVE_gcc_jit_context_new_bitcast > + > Lvalues > ------- > > diff --git a/gcc/jit/dummy-frontend.c b/gcc/jit/dummy-frontend.c > index 84ff359bfe3..c3da97642e3 100644 > --- a/gcc/jit/dummy-frontend.c > +++ b/gcc/jit/dummy-frontend.c > @@ -592,6 +592,8 @@ jit_langhook_init (void) > global_dc->begin_diagnostic = jit_begin_diagnostic; > global_dc->end_diagnostic = jit_end_diagnostic; > > + clear_nonstandard_integer_type_cache (); > + I've been putting code to cleanup global state in foo.c into foo_c_finalize functions; I'm testing a patch right now to rename these to foo_cc_finalize functions to reflect the renaming of .c to .cc These cleanup functions are called from toplev::finalize. I think it would be better to invoke this cleanup from a new tree_cc_finalize that follows that pattern. > build_common_tree_nodes (false); > > /* I don't know why this has to be done explicitly. */ [...snip...] > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index 03704ef10b8..cd8516d1c4d 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c > @@ -2405,6 +2405,34 @@ gcc_jit_context_new_cast (gcc_jit_context *ctxt, > return static_cast (ctxt->new_cast (loc, rvalue, type)); > } > > +/* Public entrypoint. See description in libgccjit.h. > + > + After error-checking, the real work is done by the > + gcc::jit::recording::context::new_bitcast method in jit-recording.c. */ > + > +gcc_jit_rvalue * > +gcc_jit_context_new_bitcast (gcc_jit_context *ctxt, > + gcc_jit_location *loc, > + gcc_jit_rvalue *rvalue, > + gcc_jit_type *type) > +{ > + RETURN_NULL_IF_FAIL (ctxt, NULL, loc, "NULL context"); > + JIT_LOG_FUNC (ctxt->get_logger ()); > + /* LOC can be NULL. */ > + RETURN_NULL_IF_FAIL (rvalue, ctxt, loc, "NULL rvalue"); > + RETURN_NULL_IF_FAIL (type, ctxt, loc, "NULL type"); > + // TODO: check the sizes. > + /*RETURN_NULL_IF_FAIL_PRINTF3 ( > + is_valid_cast (rvalue->get_type (), type), > + ctxt, loc, > + "cannot cast %s from type: %s to type: %s", > + rvalue->get_debug_string (), > + rvalue->get_type ()->get_debug_string (), > + type->get_debug_string ());*/ I belive that in general we can't do the checking for equal size until we have trees i.e. at playback time. I suspect that if the user gets this wrong, it will lead to an internal compiler error at playback time. Please can you add a testcase that tests mismatching sizes e.g. trying to interpret an int as a array of 4096 bytes, or something similar that's wildly wrong (ideally coverage for casting in both directions).   Ideally we can generate a useful error message, or, at least, not crash. > + > + return static_cast (ctxt->new_bitcast (loc, rvalue, type)); > +} > + > /* Public entrypoint. See description in libgccjit.h. > > After error-checking, the real work is done by the [...snip...] > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h > index 29afe064db6..656351edce1 100644 > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h > @@ -77,6 +77,13 @@ > /* test-builtin-unreachable.c: We don't add this one, since it touches > the optimization level of the context as a whole. */ > > +/* test-bitcast.c */ > +#define create_code create_code_bitcast > +#define verify_code verify_code_bitcast > +#include "test-bitcast.c" > +#undef create_code > +#undef verify_code > + > /* test-calling-external-function.c */ > #define create_code create_code_calling_external_function > #define verify_code verify_code_calling_external_function > @@ -400,6 +407,9 @@ const struct testcase testcases[] = { > {"builtin-memcpy", > create_code_builtin_memcpy, > verify_code_builtin_memcpy}, > + {"bitcast", > + create_code_bitcast, > + verify_code_bitcast}, > {"calling_external_function", > create_code_calling_external_function, > verify_code_calling_external_function}, > diff --git a/gcc/testsuite/jit.dg/test-bitcast.c b/gcc/testsuite/jit.dg/test-bitcast.c > new file mode 100644 > index 00000000000..2d70622051a > --- /dev/null > +++ b/gcc/testsuite/jit.dg/test-bitcast.c > @@ -0,0 +1,60 @@ > +#include > +#include > +#include > + > +#include "libgccjit.h" > + > +#include "harness.h" > + > +void > +create_code (gcc_jit_context *ctxt, void *user_data) > +{ > + /* Let's try to inject the equivalent of: > +int > +my_bitcast (double x) > +{ > + return bitcast(x, int); > +} > + */ > + gcc_jit_type *int_type = > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT); > + gcc_jit_type *double_type = > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_FLOAT); This says "double_type" but is GCC_JIT_TYPE_FLOAT. I don't think we're guaranteed that sizeof(int) == sizeof(float) on all targets. > + > + gcc_jit_param *x = > + gcc_jit_context_new_param ( > + ctxt, > + NULL, > + double_type, "x"); > + gcc_jit_param *params[1] = {x}; > + gcc_jit_function *func = > + gcc_jit_context_new_function (ctxt, > + NULL, > + GCC_JIT_FUNCTION_EXPORTED, > + int_type, > + "my_bitcast", > + 1, params, 0); > + > + gcc_jit_block *initial = > + gcc_jit_function_new_block (func, "initial"); > + > + gcc_jit_block_end_with_return(initial, NULL, > + gcc_jit_context_new_bitcast(ctxt, > + NULL, > + gcc_jit_param_as_rvalue(x), > + int_type > + )); > +} > + > +void > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > +{ > + typedef int (*my_bitcast_fn_type) (double); > + CHECK_NON_NULL (result); > + my_bitcast_fn_type my_bitcast = > + (my_bitcast_fn_type)gcc_jit_result_get_code (result, "my_bitcast"); > + CHECK_NON_NULL (my_bitcast); > + int val = my_bitcast (-5.1298714); > + note ("my_bitcast returned: %d", val); > + CHECK_VALUE (val, 35569201); > +} > diff --git a/gcc/tree.c b/gcc/tree.c > index d98b77db50b..e4bf4e84675 100644 > --- a/gcc/tree.c > +++ b/gcc/tree.c > @@ -6963,6 +6963,14 @@ build_reference_type (tree to_type) > (HOST_BITS_PER_WIDE_INT > 64 ? HOST_BITS_PER_WIDE_INT : 64) > static GTY(()) tree nonstandard_integer_type_cache[2 * MAX_INT_CACHED_PREC + 2]; > > +void clear_nonstandard_integer_type_cache (void) > +{ > + for (size_t i = 0 ; i < 2 * MAX_INT_CACHED_PREC + 2 ; i++) > + { > + nonstandard_integer_type_cache[i] = NULL; > + } > +} > + As noted above (for gcc/jit/dummy-frontend.c), make this static, and call it from a new tree_cc_finalize function, and call that from toplev::finalize. > /* Builds a signed or unsigned integer type of precision PRECISION. > Used for C bitfields whose precision does not match that of > built-in target types. */ > diff --git a/gcc/tree.h b/gcc/tree.h > index 318019c4dc5..640b492802c 100644 > --- a/gcc/tree.h > +++ b/gcc/tree.h > @@ -5385,6 +5385,7 @@ extern bool real_minus_onep (const_tree); > extern void init_ttree (void); > extern void build_common_tree_nodes (bool); > extern void build_common_builtin_nodes (void); > +extern void clear_nonstandard_integer_type_cache (void); ...and get rid of this in favor of a tree_cc_finalize. > extern tree build_nonstandard_integer_type (unsigned HOST_WIDE_INT, int); > extern tree build_nonstandard_boolean_type (unsigned HOST_WIDE_INT); > extern tree build_range_type (tree, tree, tree); > -- > 2.26.2.7.g19db9cfb68.dirty > Hope this is constructive; thanks again for the patch Dave