From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x634.google.com (mail-ej1-x634.google.com [IPv6:2a00:1450:4864:20::634]) by sourceware.org (Postfix) with ESMTPS id 20FF2389C42A for ; Tue, 3 Aug 2021 10:14:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 20FF2389C42A Received: by mail-ej1-x634.google.com with SMTP id u3so2954016ejz.1 for ; Tue, 03 Aug 2021 03:14:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=EFuBvnDqQiAtjOVzFUgvx/QFsGqishTxHnSgSwx/c9g=; b=pNcQyTo/qhK9klAkYjDsfDoza88r22Q3qTpBi1JY9/ebY+XCxvLWHBjf3K2Oa2pYly yxRUNABEtDf3okABryDWPXfxvMfrD9kO738JdXHkymcjpx36Skmwlp95GGoBrN/vyJqo 7tgWiiosCX5IDvMfF2FFP9KYeDqABVULtJos0Y8AncBprg6WlgTmtjQ5lba9IkwolaO6 3/zKVjK7+68IgGw3Ftzg7ePnw+DrSYUuJHT84PscK7ryv5phlQa/qlxbQUYvG6r+kOi+ J0/wC7Ad6dAcvm1ayIqWpAihnZI33ltPgeMHybmISJbISwyQ+zwOR7S4VM9i8di/gZCN 9YJg== X-Gm-Message-State: AOAM533LcX/xLAN/AA9eHld1ygbDC5EnnWRsR8WbQB4rQwu5zDMU1nfC jqQLRaCvifEDAigK5IQtJM0LS60EVcQR5UyW5uqHtQ== X-Google-Smtp-Source: ABdhPJwqHUwmUyvUGUr7Xs6PWFNesW/+Hgde8bMADxJiWdkyCWRwRwMrJay9KYIZEUWoH92cL46qgfnPo+ni9veUINM= X-Received: by 2002:a17:906:11d4:: with SMTP id o20mr19513040eja.329.1627985647090; Tue, 03 Aug 2021 03:14:07 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Prathamesh Kulkarni Date: Tue, 3 Aug 2021 15:43:30 +0530 Message-ID: Subject: Re: [RFC] Adding a new attribute to function param to mark it as constant To: Richard Biener Cc: Andrew Pinski , GCC Development , Richard Earnshaw Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, 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 10:14:10 -0000 On Tue, 3 Aug 2021 at 15:41, Prathamesh Kulkarni 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-en= d: > > > > #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 whic= h > > > > is generic and such; add an argument to say the intrinsics name eve= n. > > > > 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 =3D vld1_s32 (a); > > > int b =3D 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. > > 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) Oops typo, this should have been int *const 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 =E2=80=98g=E2=80=99: > test.c:7:9: error: argument 2 is not a constant. > 7 | f (0, p); > | ^ > test.c:2:6: note: Function =E2=80=98f=E2=80=99 has parameter 2 with const= eval 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, an= d 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 vecto= r > > > > > extensions where > > > > > possible (PR66791), because the builtins are opaque to the optimi= zers. > > > > > > > > > > Unfortunately, we lose type checking of immediate value if we rep= lace > > > > > 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 paramete= r 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