From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by sourceware.org (Postfix) with ESMTPS id A7BB03857016 for ; Tue, 30 May 2023 14:49:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A7BB03857016 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=ispras.ru Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=ispras.ru Received: from [10.10.3.121] (unknown [10.10.3.121]) by mail.ispras.ru (Postfix) with ESMTPS id E864740737D7; Tue, 30 May 2023 14:49:08 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru E864740737D7 Date: Tue, 30 May 2023 17:49:08 +0300 (MSK) From: Alexander Monakov To: Richard Biener cc: gcc-patches Subject: Re: [PATCH] doc: clarify semantics of vector bitwise shifts In-Reply-To: Message-ID: <8f166a4d-ec86-a95d-01d0-c381631da1df@ispras.ru> References: <4b477e8c-f8e6-5587-23ed-540f0c28ea05@ispras.ru> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323328-1926768923-1685458148=:30299" X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,KAM_SHORT,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-1926768923-1685458148=:30299 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Thu, 25 May 2023, Richard Biener wrote: > On Wed, May 24, 2023 at 8:36 PM Alexander Monakov wrote: > > > > > > On Wed, 24 May 2023, Richard Biener via Gcc-patches wrote: > > > > > I’d have to check the ISAs what they actually do here - it of course depends > > > on RTL semantics as well but as you say those are not strictly defined here > > > either. > > > > Plus, we can add the following executable test to the testsuite: > > Yeah, that's probably a good idea. I think your documentation change > with the added sentence about the truncation is OK. I am no longer confident in my patch, sorry. My claim about vector shift semantics in OpenCL was wrong. In fact it specifies that RHS of a vector shift is masked to the exact bitwidth of the element type. So, to collect various angles: 1. OpenCL semantics would need an 'AND' before a shift (except VSX/Altivec). 2. From user side we had a request to follow C integer promotion semantics in https://gcc.gnu.org/PR91838 but I now doubt we can do that. 3. LLVM makes oversized vector shifts UB both for 'vector_size' and 'ext_vector_type'. 4. Vector lowering does not emit promotions, and starting from gcc-12 ranger treats oversized shifts according to the documentation you cite below, and optimizes (e.g. with '-O2 -mno-sse') typedef short v8hi __attribute__((vector_size(16))); void f(v8hi *p) { *p >>= 16; } to zeroing '*p'. If this looks unintended, I can file a bug. I still think we need to clarify semantics of vector shifts, but probably not in the way I proposed initially. What do you think? Thanks. Alexander > Note we have > > /* Shift operations for shift and rotate. > Shift means logical shift if done on an > unsigned type, arithmetic shift if done on a signed type. > The second operand is the number of bits to > shift by; it need not be the same type as the first operand and result. > Note that the result is undefined if the second operand is larger > than or equal to the first operand's type size. > > The first operand of a shift can have either an integer or a > (non-integer) fixed-point type. We follow the ISO/IEC TR 18037:2004 > semantics for the latter. > > Rotates are defined for integer types only. */ > DEFTREECODE (LSHIFT_EXPR, "lshift_expr", tcc_binary, 2) > > in tree.def which implies short << 24 is undefined behavior (similar > wording in generic.texi). The rtl docs say nothing about behavior > but I think the semantics should carry over. That works for x86 > even for scalar instructions working on GPRs (masking is applied > but fixed to 5 or 6 bits even for QImode or HImode shifts). > > Note that when we make these shifts well-defined there's > also arithmetic on signed types smaller than int (which again > doesn't exist in C) where overflow invokes undefined behavior > in the middle-end. Unless we want to change that as well > this is somewhat inconsistent then. > > There's also the issue that C 'int' is defined by INT_TYPE_SIZE > and thus target dependent which makes what is undefined and > what not target dependent. > > Richard. > > > #include > > > > #define CHECK(TYPE, WIDTH, OP, COUNT, INVERT) \ > > { \ > > typedef TYPE vec __attribute__((vector_size(WIDTH))); \ > > \ > > static volatile vec zero; \ > > vec tmp = (zero-2) OP (COUNT); \ > > vec ref = INVERT zero; \ > > if (__builtin_memcmp(&tmp, &ref, sizeof tmp)) \ > > __builtin_abort(); \ > > } > > > > int main(void) > > { > > CHECK( uint8_t, 16, <<, 8, ) > > CHECK( uint8_t, 16, <<, 31, ) > > CHECK( uint8_t, 16, >>, 8, ) > > CHECK( uint8_t, 16, >>, 31, ) > > CHECK( int8_t, 16, <<, 8, ) > > CHECK( int8_t, 16, <<, 31, ) > > CHECK( int8_t, 16, >>, 8, ~) > > CHECK( int8_t, 16, >>, 31, ~) > > CHECK(uint16_t, 16, <<, 16, ) > > CHECK(uint16_t, 16, <<, 31, ) > > CHECK(uint16_t, 16, >>, 16, ) > > CHECK(uint16_t, 16, >>, 31, ) > > CHECK( int16_t, 16, <<, 16, ) > > CHECK( int16_t, 16, <<, 31, ) > > CHECK( int16_t, 16, >>, 16, ~) > > CHECK( int16_t, 16, >>, 31, ~) > > // Per-lane-variable shifts: > > CHECK( uint8_t, 16, <<, zero+8, ) > > CHECK( uint8_t, 16, <<, zero+31, ) > > CHECK( uint8_t, 16, >>, zero+8, ) > > CHECK( uint8_t, 16, >>, zero+31, ) > > CHECK( int8_t, 16, <<, zero+8, ) > > CHECK( int8_t, 16, <<, zero+31, ) > > CHECK( int8_t, 16, >>, zero+8, ~) > > CHECK( int8_t, 16, >>, zero+31, ~) > > CHECK(uint16_t, 16, <<, zero+16, ) > > CHECK(uint16_t, 16, <<, zero+31, ) > > CHECK(uint16_t, 16, >>, zero+16, ) > > CHECK(uint16_t, 16, >>, zero+31, ) > > CHECK( int16_t, 16, <<, zero+16, ) > > CHECK( int16_t, 16, <<, zero+31, ) > > CHECK( int16_t, 16, >>, zero+16, ~) > > CHECK( int16_t, 16, >>, zero+31, ~) > > > > // Repeat for WIDTH=32 and WIDTH=64 > > } > > > > Alexander > --8323328-1926768923-1685458148=:30299--