From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id D75B53858032 for ; Tue, 6 Jun 2023 17:03:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D75B53858032 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 31C932F4; Tue, 6 Jun 2023 10:04:25 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 091D83F663; Tue, 6 Jun 2023 10:03:38 -0700 (PDT) From: Richard Sandiford To: Oluwatamilore Adebayo Mail-Followup-To: Oluwatamilore Adebayo ,, , richard.sandiford@arm.com Cc: , Subject: Re: [PATCH] rtl: AArch64: New RTL for ABD References: <20230606151153.53653-1-oluwatamilore.adebayo@arm.com> Date: Tue, 06 Jun 2023 18:03:37 +0100 In-Reply-To: <20230606151153.53653-1-oluwatamilore.adebayo@arm.com> (Oluwatamilore Adebayo's message of "Tue, 6 Jun 2023 16:11:53 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-22.0 required=5.0 tests=BAYES_00,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Oluwatamilore Adebayo writes: >> It would be good to mark all of these functions with __attribute__((noipa)), >> since I think interprocedural optimisations might otherwise defeat the >> runtime test in abd_run_1.c (in the sense that we might end up folding >> things at compile time and not testing the vector versions of the functions). > > Done. > >> There are 14 tests, and it looks like 6 of them are expected to produce >> ABD instructions while 8 aren't. It isn't really clear which tests are >> which though. >> >> I think it'd help to split the file into two: >> >> - one containing only the tests that should produce ABD, so that the >> scan-assembler counts sum up to the number of tests >> >> - one containing only the tests that cannot use ABD, with: >> >> { dg-final { scan-assembler-not {\tsabd\t} } } >> { dg-final { scan-assembler-not {\tuabd\t} } } >> >> to enforce that > > After adjustments made to the vectoriser part, all tests now use an abd > instruction. Ah, that's a problem. Sorry, I didn't review 1/2 closely enough. For: > + /* Failed to find a widen operation so we check for a regular MINUS_EXPR. */ > + if (diff > + && gimple_assign_rhs_code (diff) == MINUS_EXPR > + && (TYPE_UNSIGNED (abs_type) || TYPE_OVERFLOW_UNDEFINED (abs_type))) > + { > + *half_type = NULL_TREE; > + return true; > + } the condition should instead be: if (diff && gimple_assign_rhs_code (diff) == MINUS_EXPR && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (abs_oprnd))) { *half_type = NULL_TREE; return true; } That is, we rely on overflow being undefined, so we need to check TYPE_OVERFLOW_UNDEFINED on the type of the subtraction (rather than abs_type, which is the type of ABS input, and at this point can be different from TREE_TYPE (abs_oprnd)). Then fn_unsigned_int and fn_unsigned_char_int_short correctly avoid using SABD. The output for the other tests looks right. It would be good to add a: /* { dg-final { scan-assembler-not {\tabs\t} } } */ to be the positive tests, to make it more obvious that all separate ABS instructions are elided. Thanks, Richard