From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72d.google.com (mail-qk1-x72d.google.com [IPv6:2607:f8b0:4864:20::72d]) by sourceware.org (Postfix) with ESMTPS id 095123851C14 for ; Tue, 3 Aug 2021 17:44:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 095123851C14 Received: by mail-qk1-x72d.google.com with SMTP id o13so119000qkk.9 for ; Tue, 03 Aug 2021 10:44:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=hBA/xoAa0T0UOe8WQeCXcbXPV0hhuxBYh9pFV+yUcXA=; b=KqT/USUR4eOkLBuVWT/Zoi5X7z/sJX3HwQj2zwmMKE7yLIy6SGIspseVrAPnxhkQaP RSBXiBAGIR1AHuWg8Hf8GPd3BVeuyqNXFDgu1VluUQpt6M+Roe6i40zf037EYjZcUR/Q Alr8/z0v9vWhPkXtUGUIYIry6X5Cbp+otLx5NZ9Xqwutm2Sgx4OkM86x7eYVzvIQu6EY +6q5vYN4LOGJ1ATmu3rvzZkn12D7iGLmtDCjZB4ssQmKwwHmrLhDMm2iAKS4Yu5RRcre r1uUcK9cvQQcMRfHb3CJB3Wx1O1jjo5sygx5Q15G7CvDTk6j9o84evA5ZzHKe6Oy+htz 1EdQ== X-Gm-Message-State: AOAM531pXQapvUCz30pyHdCzPNw65UFfeK3qbaI34LanAPpdcG1WVm4I GaiAUpphRjXUfqNwvMQeIhc= X-Google-Smtp-Source: ABdhPJz5S9aZc5cKuncad1/iMd8pXA1/He6kTSP+icu4+fLWeGxzeLA2QisZ3Q7awmCbCmm+RX53hQ== X-Received: by 2002:a37:a252:: with SMTP id l79mr21672929qke.128.1628012655592; Tue, 03 Aug 2021 10:44:15 -0700 (PDT) Received: from [192.168.0.41] (75-166-102-22.hlrn.qwest.net. [75.166.102.22]) by smtp.gmail.com with ESMTPSA id a5sm8142836qkf.88.2021.08.03.10.44.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 Aug 2021 10:44:15 -0700 (PDT) Subject: Re: [RFC] Adding a new attribute to function param to mark it as constant To: Prathamesh Kulkarni , Richard Biener Cc: GCC Development , Richard Earnshaw References: From: Martin Sebor Message-ID: <9587be75-ec2a-a997-3074-1811228245d3@gmail.com> Date: Tue, 3 Aug 2021 11:44:13 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Aug 2021 17:44:17 -0000 On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: > On Tue, 27 Jul 2021 at 13:49, Richard Biener wrote: >> >> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc >> wrote: >>> >>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski wrote: >>>> >>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc >>>> wrote: >>>>> >>>>> Hi, >>>>> Continuing from this thread, >>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html >>>>> The proposal is to provide a mechanism to mark a parameter in a >>>>> function as a literal constant. >>>>> >>>>> Motivation: >>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: >>>>> >>>>> __extension__ extern __inline int32x2_t >>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>> { >>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>> } >>>>> >>>>> and it's caller: >>>>> >>>>> int32x2_t f (int32x2_t x) >>>>> { >>>>> return vshl_n_s32 (x, 1); >>>>> } >>>> >>>> Can't you do similar to what is done already in the aarch64 back-end: >>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) >>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ >>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), >>>> sizeof(__vec[0]), __idx) >>>> >>>> ? >>>> Yes this is about lanes but you could even add one for min/max which >>>> is generic and such; add an argument to say the intrinsics name even. >>>> You could do this as a non-target builtin if you want and reuse it >>>> also for the aarch64 backend. >>> Hi Andrew, >>> Thanks for the suggestions. IIUC, we could use this approach to check >>> if the argument >>> falls within a certain range (min / max), but I am not sure how it >>> will help to determine >>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require >>> that the 2nd arg is immediate ? >>> >>> Even the current RTL builtin checking is not consistent across >>> optimization levels: >>> For eg: >>> int32x2_t f(int32_t *restrict a) >>> { >>> int32x2_t v = vld1_s32 (a); >>> int b = 2; >>> return vshl_n_s32 (v, b); >>> } >>> >>> With pristine trunk, compiling with -O2 results in no errors because >>> constant propagation replaces 'b' with 2, and during expansion, >>> expand_builtin_args is happy. But at -O0, it results in the error - >>> "argument 2 must be a constant immediate". >>> >>> So I guess we need some mechanism to mark a parameter as a constant ? >> >> I guess you want to mark it in a way that the frontend should force >> constant evaluation and error if that's not possible? C++ doesn't >> allow to declare a parameter as 'constexpr' but something like >> >> void foo (consteval int i); >> >> since I guess you do want to allow passing constexpr arguments >> in C++ or in C extended forms of constants like >> >> static const int a[4]; >> >> foo (a[1]); >> >> ? But yes, this looks useful to me. > Hi Richard, > Thanks for the suggestions and sorry for late response. > I have attached a prototype patch that implements consteval attribute. > As implemented, the attribute takes at least one argument(s), which > refer to parameter position, > and the corresponding parameter must be const qualified, failing > which, the attribute is ignored. I'm curious why the argument must be const-qualified. If it's to keep it from being changed in ways that would prevent it from being evaluated at compile-time in the body of the function then to be effective, the enforcement of the constraint should be on the definition of the function. Otherwise, the const qualifier could be used in a declaration of a function but left out from a subsequent definition of it, letting it modify it, like so: __attribute__ ((consteval (1))) void f (const int); inline __attribute__ ((always_inline)) void f (int i) { ++i; } That said, if compile-time function evaluation is the goal then a fully general solution is an attribute that applies to the whole function, not just a subset of its arguments. That way arguments can also be assigned to local variables within the function that can then be modified while still evaluated at compile time and used where constant expressions are expected. I.e., the design goal is [a subset of] C++ constexpr. (Obviously a much bigger project.) A few notes on the prototype patch: conventionally GCC warnings about attributes do not mention when an attribute is ignored. It may be a nice touch to add to all of them but I'd recommend against doing that in individual handlers. Since the attribute allows pointer constants the warning issued when an argument is not one should be generalized (i.e., not refer to just integer constants). (Other than that, C/C++ warnings should start in lowercase and not end in a period). Martin > > The patch does type-checking for arguments in > check_function_consteval_attr, which > simply does a linear search to see if the corresponding argument > number is included in consteval attribute, > and if yes, it checks if the argument is CONSTANT_CLASS_P. > > This works for simple constants and the intrinsics use-case, but > rejects "extended constants" as in your above example. > I guess we can special case to detect cases like a[i] where 'a' is > const and 'i' is an immediate, > but I am not sure if there's a way to force constant evaluation in FE ? > I tried using c_fully_fold (argarray[i], false, &maybe_const) but that > didn't seem to work. > Do you have any suggestions on how to detect those in the front-end ? > > Example: > > __attribute__((consteval(1, 2))) > void f(const int x, int *p) > {} > > Calling it with: > 1) f(0, (int *) 0) is accepted. > > 2) > void g(int *p) > { > f (0, p); > } > > emits the following error: > test.c: In function ‘g’: > test.c:7:9: error: argument 2 is not a constant. > 7 | f (0, p); > | ^ > test.c:2:6: note: Function ‘f’ has parameter 2 with consteval attribute. > 2 | void f(const int x, int *const p) > | ^ > > Thanks, > Prathamesh >> >> Richard. >> >>> >>> Thanks, >>> Prathamesh >>>> >>>> Thanks, >>>> Andrew Pinski >>>> >>>>> >>>>> The constraint here is that, vshl_n intrinsics require that the >>>>> second arg (__b), >>>>> should be an immediate value. >>>>> Currently, this check is performed by arm_expand_builtin_args, and if >>>>> a non-constant >>>>> value gets passed, it emits the following diagnostic: >>>>> >>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must >>>>> be a constant immediate >>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> >>>>> However, we're trying to replace builtin calls with gcc's C vector >>>>> extensions where >>>>> possible (PR66791), because the builtins are opaque to the optimizers. >>>>> >>>>> Unfortunately, we lose type checking of immediate value if we replace >>>>> the builtin >>>>> with << operator: >>>>> >>>>> __extension__ extern __inline int32x2_t >>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>> { >>>>> return __a << __b; >>>>> } >>>>> >>>>> So, I was wondering if we should have an attribute for a parameter to >>>>> specifically >>>>> mark it as a constant value with optional range value info ? >>>>> As Richard suggested, sth like: >>>>> void foo(int x __attribute__((literal_constant (min_val, max_val))); >>>>> >>>>> Thanks, >>>>> Prathamesh