From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 6C85F3858439; Mon, 24 Oct 2022 13:29:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6C85F3858439 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 5D6FF1FD56; Mon, 24 Oct 2022 13:29:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1666618180; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Jof2oOYhROWFdrm0/tj8S0Dx8c90CZY4BtxOJC///fQ=; b=1GY/n8uNWDoX4/i+wHXv/74Hyodg+0FC1/xoE+vhDjgQ+nGpKUqTGyuyc6eKlLuDOL5pMW ifxipUYy/FvbY8d16Go1HFQuxOobBOc7C3+07MDpqegvmz5jo9f3AowPpT1bQHtrHYe3LH nNuq5JYcZK9Qnc5TvsXXMsrf8BMeygY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1666618180; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Jof2oOYhROWFdrm0/tj8S0Dx8c90CZY4BtxOJC///fQ=; b=fJu5T73EBM9j2to7mX9WTZ3vxKVti444nbfBdmqW5TKN2loNrOLoKkiMu4rko9khbZWEsA NFKpOl+SHKD2oaBw== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 50CFD2C141; Mon, 24 Oct 2022 13:29:40 +0000 (UTC) Date: Mon, 24 Oct 2022 13:29:40 +0000 (UTC) From: Richard Biener To: "Andre Vieira (lists)" cc: "gcc-patches@gcc.gnu.org" , Richard Sandiford , ebotcazou@gcc.gnu.org Subject: Re: vect: Make vect_check_gather_scatter reject offsets that aren't multiples of BITS_PER_UNIT [PR107346] In-Reply-To: <3cc86b91-eef3-7def-7b3b-464c58369f4b@arm.com> Message-ID: References: <129db1b0-0d2a-b768-bc80-9f73d665e8f8@arm.com> <3cc86b91-eef3-7def-7b3b-464c58369f4b@arm.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="-1609957120-1974043427-1666618180=:4294" X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP 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. ---1609957120-1974043427-1666618180=:4294 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Mon, 24 Oct 2022, Andre Vieira (lists) wrote: > > On 24/10/2022 13:46, Richard Biener wrote: > > On Mon, 24 Oct 2022, Andre Vieira (lists) wrote: > > > >> On 24/10/2022 08:17, Richard Biener wrote: > >>> Can you check why vect_find_stmt_data_reference doesn't trip on the > >>> > >>> if (TREE_CODE (DR_REF (dr)) == COMPONENT_REF > >>> && DECL_BIT_FIELD (TREE_OPERAND (DR_REF (dr), 1))) > >>> { > >>> free_data_ref (dr); > >>> return opt_result::failure_at (stmt, > >>> "not vectorized:" > >>> " statement is an unsupported" > >>> " bitfield access %G", stmt); > >>> } > >> It used to, which is why this test didn't trigger the error before my > >> patch, > >> but we lower it to BIT_FIELD_REFs in ifcvt now so it is no longer a > >> DECL_BIT_FIELD. > >> > >> But that is a red-herring, if you change the test structure's 'type Int24 > >> is > >> mod 2**24;' to 'type Int24 is mod 2**32;', thus making the field we access > >> a > >> normal 32-bit integer, the field no longer is a DECL_BIT_FIELD and thus my > >> lowering does nothing. However, you will still get the failure because the > >> field before it is a packed 4-bit field, making the offset to the field we > >> are > >> accessing less than BITS_PER_UNIT. > > Hmm, so the _intent_ of DECL_BIT_FIELD_REPRESENTATIVE is to definitely > > _not_ be a DECL_BIT_FIELD (well, that's the whole point!). So this > > shows an issue with setting up DECL_BIT_FIELD_REPRESENTATIVE? Of course > > for a type with an alignment less than BITS_PER_UNIT (is StructB actually > > such a type?) there cannot be a representative that isn't, so maybe > > we should then set DECL_BIT_FIELD on it with a condition like Eric > > mentions? > I could do this, but it would not resolve the latent issue as I could still > reproduce it without using any of the bitfield lowering code, see below. > > > >>> ? I think we should amend this check and I guess that > >>> checking multiple_p on DECL_FIELD_BIT_OFFSET should be enough? > >> That won't work either, unless we do the same walk-through the full access > >> as > >> we do in get_inner_reference. > > I suppose we should not "if-convert" bit field accesses with a > > DECL_BIT_FIELD representative. There isn't any benefit doing that > > (not for general bitfield lowering either). > Changing if-convert would merely change this testcase but we could still > trigger using a different structure type, changing the size of Int24 to 32 > bits rather than 24: > package Loop_Optimization23_Pkg is >   type Nibble is mod 2**4; >   type Int24  is mod 2**32;  -- Changed this from 24->32 >   type StructA is record >     a : Nibble; >     b : Int24; >   end record; >   pragma Pack(StructA); >   type StructB is record >     a : Nibble; >     b : StructA; >   end record; >   pragma Pack(StructB); >   type ArrayOfStructB is array(0..100) of StructB; >   procedure Foo (X : in out ArrayOfStructB); > end Loop_Optimization23_Pkg; > > This would yield a DR_REF (dr): (*x_7(D))[_1].b.b  where the last 'b' isn't a > DECL_BIT_FIELD anymore, but the first one still is and still has the > non-multiple of BITS_PER_UNIT offset. Thus passing the > vect_find_stmt_data_reference check and triggering the > vect_check_gather_scatter failure. So unless we go and make sure we always set > the DECL_BIT_FIELD on all subsequent accesses of a DECL_BIT_FIELD 'struct' > (which is odd enough on its own) then we are better off catching the issue in > vect_check_gather_scatter ? But it's not only an issue with scatter-gather, other load/store handling assumes it can create a pointer to the start of the access and thus requires BITS_PER_UNIT alignment for each of them. So we need to fail at data-ref analysis somehow. Richard. ---1609957120-1974043427-1666618180=:4294--