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 4EBE83858D32 for ; Mon, 27 Feb 2023 14:29:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4EBE83858D32 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 308EBC14; Mon, 27 Feb 2023 06:30:37 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.99.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7625A3F67D; Mon, 27 Feb 2023 06:29:53 -0800 (PST) From: Richard Sandiford To: "Victor L. Do Nascimento" Mail-Followup-To: "Victor L. Do Nascimento" ,, , richard.sandiford@arm.com Cc: , Subject: Re: [PATCH] constraint: fix relaxed memory and repeated constraint handling References: Date: Mon, 27 Feb 2023 14:29:52 +0000 In-Reply-To: (Victor L. Do Nascimento's message of "Thu, 16 Feb 2023 14:27:46 +0000") 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=-34.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,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: "Victor L. Do Nascimento" writes: > The function `constrain_operands' lacked the logic to consider relaxed > memory constraints when "traditional" memory constraints were not > satisfied, creating potential issues as observed during the reload > compilation pass. > > In addition, it was observed that while `constrain_operands' chooses > to disregard constraints when more than one alternative is provided, > e.g. "m,r" using CONSTRAINT__UNKNOWN, it has no checks in place to > determine whether the multiple constraints in a given string are in > fact repetitions of the same constraint and should thus in fact be > treated as a single constraint, as ought to be the case for something > like "m,m". > > Both of these issues are dealt with here, thus ensuring that we get > appropriate pattern matching. > > Tested on aarch64-linux-gnu & x86_64-linux-gnu. OK to install? > > Victor > > gcc/ > * lra-constraints.cc (constraint_unique): New. > (process_address_1): Apply constraint_unique test. > * recog.cc (constrain_operands): Allow relaxed memory > constaints. > --- > gcc/lra-constraints.cc | 43 +++++++++++++++++++++++++++++++++++++++--- > gcc/recog.cc | 3 ++- > 2 files changed, 42 insertions(+), 4 deletions(-) > > diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc > index dbfaf0485..c9c1653c0 100644 > --- a/gcc/lra-constraints.cc > +++ b/gcc/lra-constraints.cc > @@ -3448,6 +3448,45 @@ skip_constraint_modifiers (const char *str) > } > } > > +/* Takes a string of 0 or more comma-separated constraints and the > + constraint_num correspondig to the first constraint. When more > + than one constraint present, evaluate whether they all correspond > + to a single, repeated constraint (e.g. "r,r") or whether we have > + more than one distinct constraints (e.g. "r,m"). */ Minor formatting nit: indentation should be to "/* " rather than "/* ". > +static bool > +constraint_unique (const char *cstr, enum constraint_num ca) > +{ > + enum constraint_num cb; > + for (;;) > + { > + /* Skip past current constraint and any whitespace which may > + precede the end-of-line or separator characters. */ > + cstr = skip_constraint_modifiers (cstr > + + CONSTRAINT_LEN (cstr[0], cstr)); > + /* If end of string reached and no disagreement found, we have > + uniqueness. */ > + if (*cstr == '\0') > + return true; > + /* skip_constraint_modifiers does not handle commas, handle > + case manually. */ > + if (*cstr == ',') > + cstr++; > + /* Get next constraint. */ > + cstr = skip_constraint_modifiers (cstr); > + cb = lookup_constraint ((*cstr == '\0' || *cstr == ',') ? "X" : cstr); > + > + /* If mismatch found, break out of loop. */ > + if (cb != ca) > + return false; > + > + /* If *cstr == '\0', we don't want to reach the > + skip_constraint_modifiers statement again as that will > + advance the pointer past the end of the string. */ > + if (*cstr == '\0') > + return true; > + } > +} How about rearranging this a bit to something like: ca = CONSTRAINT__UNKNOWN; for (;;) { cstr = skip_constraint_modifiers (cstr); if (*cstr == '\0' || *cstr == ',') cb = CONSTRAINT_X; else { cb = lookup_constraint (cstr); if (cb == CONSTRAINT__UNKNOWN) return false; cstr += CONSTRAINT_LEN (cstr[0], cstr); } if (ca == CONSTRAINT__UNKNOWN) ca = cb else if (ca != cb) return false; if (*cstr == '\0') return true; if (*cstr == ',') cstr += 1; } That way we only do one lookup per loop iteration. It also avoids CONSTRAINT_LEN for the "empty, followed by comma" case. If that works, the patch is OK with those changes once another approved-for-GCC-13 patch needs it. OK for GCC 14 otherwise. Thanks, Richard > + > /* Major function to make reloads for an address in operand NOP or > check its correctness (If CHECK_ONLY_P is true). The supported > cases are: > @@ -3507,9 +3546,7 @@ process_address_1 (int nop, bool check_only_p, > operand has one address constraint, probably all others constraints are > address ones. */ > if (constraint[0] != '\0' && get_constraint_type (cn) != CT_ADDRESS > - && *skip_constraint_modifiers (constraint > - + CONSTRAINT_LEN (constraint[0], > - constraint)) != '\0') > + && !constraint_unique (constraint, cn)) > cn = CONSTRAINT__UNKNOWN; > if (insn_extra_address_constraint (cn) > /* When we find an asm operand with an address constraint that > diff --git a/gcc/recog.cc b/gcc/recog.cc > index 200cf4214..3ddeab59d 100644 > --- a/gcc/recog.cc > +++ b/gcc/recog.cc > @@ -3234,7 +3234,8 @@ constrain_operands (int strict, alternative_mask alternatives) > else if (constraint_satisfied_p (op, cn)) > win = 1; > > - else if (insn_extra_memory_constraint (cn) > + else if ((insn_extra_memory_constraint (cn) > + || insn_extra_relaxed_memory_constraint (cn)) > /* Every memory operand can be reloaded to fit. */ > && ((strict < 0 && MEM_P (op)) > /* Before reload, accept what reload can turn