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 5A4B83858C2F for ; Tue, 16 Aug 2022 08:26:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5A4B83858C2F Received: by mail-ej1-x634.google.com with SMTP id kb8so17600363ejc.4 for ; Tue, 16 Aug 2022 01:26:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc; bh=JQ0JJBRtIZ1o3/fY5PfBCyw3NVAxR9McLKF4GLyY8sk=; b=5gGiO00DE1nLJ1FQqSkBUKeZymej2hIG5aV013yLZ+7ya2tSV+3uBthRGOuc3J1Dhx lzsAKXdO81oLrGVlDzfCVI9OdzYb+Iu6Vug5Ae6ZFsOXaQjqsf5dKIUzwXkLz682FPPT E+K1hYjcHmwceomBc/FgY41BeanSLIQ6Ze85KGoYSsSNCFd9N5uoLl8DtWMXBkVFZ11P SpgRK41iMVDFhKQynLIS/wr0C4S/Y0BpfgHLGJY/yBfwniInMLLQbEkl96p49tBmHAFE kIfMMBDU33OE4asM08dxoYrilKCRCV0pp7qmAxBqqtzGPUtxrmr3cN/ecaa9Fd+u/o+1 BNeQ== X-Gm-Message-State: ACgBeo10a2Bv5cHCrA7bGk4S7/Rweq3S+7eV9mteH1Mz7S/ysI0oUjFq NbzdCyI+W+MnWzwr9s9qfv3D6k+ht1OO377ctOXt2rvl X-Google-Smtp-Source: AA6agR7izoSHu+X4gMwsy/Kncad4kPNf/SPlssWEl6SoLbmVkAXB5RU8XeO5jEhALlvnWv0YuIYS2e2udz3Ui/dRexw= X-Received: by 2002:a17:907:3e12:b0:738:fd2f:df80 with SMTP id hp18-20020a1709073e1200b00738fd2fdf80mr764285ejc.29.1660638387198; Tue, 16 Aug 2022 01:26:27 -0700 (PDT) MIME-Version: 1.0 References: <002201d8ae8b$d11fa9b0$735efd10$@nextmovesoftware.com> In-Reply-To: From: Richard Biener Date: Tue, 16 Aug 2022 10:26:15 +0200 Message-ID: Subject: Re: [x86 PATCH] PR target/106577: force_reg may clobber operands during split. To: Richard Biener via Gcc-patches , Roger Sayle , Richard Biener , Richard Sandiford Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Aug 2022 08:26:32 -0000 On Tue, Aug 16, 2022 at 10:14 AM Richard Sandiford wrote: > > Richard Biener via Gcc-patches writes: > > On Fri, Aug 12, 2022 at 10:41 PM Roger Sayle wrote: > >> > >> > >> This patch fixes PR target/106577 which is a recent ICE on valid regression > >> caused by my introduction of a *testti_doubleword pre-reload splitter in > >> i386.md. During the split pass before reload, this converts the virtual > >> *testti_doubleword into an *andti3_doubleword and *cmpti_doubleword, > >> checking that any immediate operand is a valid "x86_64_hilo_general_operand" > >> and placing it into a TImode register using force_reg if it isn't. > >> > >> The unexpected behaviour (that caught me out) is that calling force_reg > >> may occasionally clobber the contents of the global operands array, or > >> more accurately recog_data.operand[0], which means that by the time > >> split_XXX calls gen_split_YYY the replacement insn's operands have been > >> corrupted. > >> > >> It's difficult to tell who (if anyone is at fault). The re-entrant > >> stack trace (for the attached PR) looks like: > >> > >> gen_split_203 (*testti_doubleword) calls > >> force_reg calls > >> emit_move_insn calls > >> emit_move_insn_1 calls > >> gen_movti calls > >> ix86_expand_move calls > >> ix86_convert_const_wide_int_to_broadcast calls > >> ix86_vector_duplicate_value calls > >> recog_memoized calls > >> recog. > >> > >> By far the simplest and possibly correct fix is rather than attempt > >> to push and pop recog_data, to simply (in pre-reload splits) save a > >> copy of any operands that will be needed after force_reg, and use > >> these copies afterwards. Many pre-reload splitters avoid this issue > >> using "[(clobber (const_int 0))]" and so avoid gen_split_YYY functions, > >> but in our case we still need to save a copy of operands[0] (even if we > >> call emit_insn or expand_* ourselves), so we might as well continue to > >> use the conveniently generated gen_split. > >> > >> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap > >> and make -k check, both with and without --target_board=unix{-m32}, > >> with no new failures. Ok for mainline? > > > > Why this obviously fixes the issue seen I wonder whether there's > > more of recog_data that might be used after control flow returns > > to recog_memoized and thus the fix would be there, not in any > > backend pattern triggering the issue like this? > > > > The "easiest" fix would maybe to add a in_recog flag and > > simply return FAIL from recog when recursing. Not sure what > > the effect on this particular pattern would be though? > > > > The better(?) fix might be to push/pop recog_data in 'recog', but > > of course give that recog_data is currently a global leakage > > in intermediate code can still happen. > > > > That said - does anybody know of similar fixes for this issue in other > > backends patterns? > > I don't think it's valid for a simple query function like > ix86_vector_duplicate_value to clobber global state. Doing that > could cause problems in other situations, not just splits. > > Ideally, it would be good to wean insn-recog.cc:recog off global state. > The only parts of recog_data it uses (if I didn't miss something) > are recog_data.operands and recog_data.insn (but only to nullify > it for recog_memoized, which wouldn't be necessary if recog didn't > clobber recog_data.operands). But I guess some .md expand/insn > conditions probably rely on the operands array being in recog_data, > so that might not be easy. > > IMO the correct low-effort fix is to save and restore recog_data > in ix86_vector_duplicate_value. It's a relatively big copy, > but the current code is pretty wasteful anyway (allocating at > least a new SET and INSN for every query). Compared to the > overhead of doing that, a copy to and from the stack shouldn't > be too bad. I see. I wonder if we should at least add some public API for save/restore of recog_data so the many places don't need to invent their own version and they are more easily to find later. Maybe some RAII { push_recog_data saved (); } ? Shall we armor recog () for recursive invocation by adding a ->in_recog member to recog_data? > > Thanks, > Richard