From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd35.google.com (mail-io1-xd35.google.com [IPv6:2607:f8b0:4864:20::d35]) by sourceware.org (Postfix) with ESMTPS id EDB5E3858D1E for ; Fri, 10 Nov 2023 21:48:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EDB5E3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EDB5E3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::d35 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699652923; cv=none; b=DMvpCp3VCBhfPl3FLuGZGVZuUYaRMbmVh3A/8+nUnfNuwT3Eymj1CfILQUZ3/YaJrJKQTAtoFI8vV722GFm+WFfCmGq3suiiKSELrWkVPn4H+gK/BMhkcq2fC7eu4XPIE+fC5XqI33jMEvrr6lbI0kDXxO9pcJeVd785rrX84As= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699652923; c=relaxed/simple; bh=xigBIk20lA/MZzvwMLqfUAkT2iDwr3wbsz2mVUI1w/A=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=nqsC7KETf9GTqXmbYZqeawHEjyDD3MM55hvMyn7f1QbP31zwQusSRcKJDBlapQG/Z38V6Zz0P1B4Mkq83DOblKfRgE8nbPMUpQKe8HfN4wIYhQRvKLUHZNmQy6yvsaRQs6QEaUMqzwqqYjTtQTFoBt+ErxdfTibwFzGBXPTyDtk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-io1-xd35.google.com with SMTP id ca18e2360f4ac-7b006f7c473so32648839f.2 for ; Fri, 10 Nov 2023 13:48:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1699652920; x=1700257720; darn=gcc.gnu.org; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=lJchN+UfKD2DavY/0D8qN31LtsInLyknZ//orZPJXC0=; b=dvDjJoPmsAO329Baj/m4x4ufNmVr96/KOai/r1pCMTrt5j4tydg9XBbsAzLNg8raDv ndlIkH86C1g1TQcp9C/6IUH5qIrrsEN8PcWnuqz43QMUvJuAazmzUNU3rV/1uQ63/g6p li3xMnwExncPivQknuOTmX0kl//fz7ef13plbxTuoOlJnjjZlS9E6fewnY/oTqElCOQ1 z7kv4ulJS8nwmA7XeVitMgrl+Ukavs0vfc9yXLuSu11qgec8DVTBQg+mRS1mcebBY3tK O02DRGz3dnCbtIp7lcIA6tR1vU/xnB+dvC/js1MDokoCAvqWJ2NXfcJIbWvQWGB5DbZD aKyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699652920; x=1700257720; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=lJchN+UfKD2DavY/0D8qN31LtsInLyknZ//orZPJXC0=; b=T+JM81DpEux15OgIWGjJrE44gyQ8O32ak2730M2GDhv8BptsPwCWroSDDzgg0YJVFK EMZYel6OZgI7l2n7J0Ut5rQ7cSho206dIo4AG7A3N2uNH2iMu6QrevBUMvcJ4ULVkCoC ydvbEsgKNyUQZfAj8JAr7PuOw5hq+JWYPFOEb/DTRQ8TAJDqidLHUGCWBI+DjnUvkpCE S53tWVIiDVMDaeRi2A1mo4LBbl61yCLVS7Gvi7Xf55oFNX5tiZUH03x+CO2TcDUnNnqJ mm5IrghnW1uAtds+ZwUteY2cYH76kCLh4HJRQEvUokcxWvCs95K5tMW4tiqxMpH53+Rr lXjQ== X-Gm-Message-State: AOJu0YwJB0qgPYW9tgqWcWDOh9mFvvObHrvGgwGHUCZo6VaFkZMWg9Cw C+wmOitVE6InE2tMxH7qS0YK4Ok8k7OgKA== X-Google-Smtp-Source: AGHT+IGRB2emqmS3cSrycYEV76SlNJPGgDVOcA0jTTHVJrZ8lCFzJlayp/kVv8Eiyzqc3IENFyx+kg== X-Received: by 2002:a6b:c945:0:b0:7aa:125a:b0c4 with SMTP id z66-20020a6bc945000000b007aa125ab0c4mr654063iof.5.1699652920021; Fri, 10 Nov 2023 13:48:40 -0800 (PST) Received: from [172.31.0.109] ([136.36.130.248]) by smtp.gmail.com with ESMTPSA id e91-20020a028664000000b0043a21abd491sm80545jai.120.2023.11.10.13.48.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 10 Nov 2023 13:48:39 -0800 (PST) Message-ID: <8184e869-b641-47a6-b5a3-090c1ff00972@gmail.com> Date: Fri, 10 Nov 2023 14:48:38 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/4] ifcvt: handle sequences that clobber flags in noce_convert_multiple_sets Content-Language: en-US To: Robin Dapp , Manolis Tsamis , gcc-patches@gcc.gnu.org, Philipp Tomsich , Jakub Jelinek , richard.sandiford@arm.com References: <20230830101400.1539313-1-manolis.tsamis@vrull.eu> <20230830101400.1539313-2-manolis.tsamis@vrull.eu> <6cf3deca-2edc-419a-a66c-b987324e3da9@gmail.com> From: Jeff Law In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-2.3 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 List-Id: On 10/20/23 03:16, Richard Sandiford wrote: > Thanks for the context. > > Robin Dapp writes: >>> Sorry for the slow review. TBH I was hoping someone else would pick >>> it up, since (a) I'm not very familiar with this code, and (b) I don't >>> really agree with the way that the current code works. I'm not sure the >>> current dependency checking is safe, so I'm nervous about adding even >>> more cases to it. And it feels like the different ifcvt techniques are >>> not now well distinguished, so that they're beginning to overlap and >>> compete with one another. None of that is your fault, of course. :) >> >> I might be to blame, at least partially :) The idea back then was to >> do it here because (1) it can handle cases the other part cannot and >> (2) its costing is based on sequence cost rather than just counting >> instructions. > > Ah, OK. (2) seems like a good reason. Agreed. It's been a problem area (costing ifcvt), but it's still the right thing to do. No doubt if we change something from counting insns to sequence costing it'll cause another set of problems, but that shouldn't stop us from doing the right thing here. > > Yeah, makes sense. Using your example, there seem to be two things > that we're checking: > > (1) Does the sequence change cc? This is checked via: > > if (cc_cmp) > { > /* Check if SEQ can clobber registers mentioned in > cc_cmp and/or rev_cc_cmp. If yes, we need to use > only seq1 from that point on. */ > rtx cc_cmp_pair[2] = { cc_cmp, rev_cc_cmp }; > for (walk = seq; walk; walk = NEXT_INSN (walk)) > { > note_stores (walk, check_for_cc_cmp_clobbers, cc_cmp_pair); > if (cc_cmp_pair[0] == NULL_RTX) > { > cc_cmp = NULL_RTX; > rev_cc_cmp = NULL_RTX; > break; > } > } > } > > and is the case that Manolis's patch is dealing with. > > (2) Does the sequence use a and b? If so, we need to use temporary > destinations for any earlier writes to a and b. > > Is that right? > > (1) looks OK, although Manolis's modified_in_p would work too. Agreed. > (2) is the code I quoted yesterday and is the part I'm not sure > about. First of all: > > seq1 = try_emit_cmove_seq (if_info, temp, cond, > new_val, old_val, need_cmov, > &cost1, &temp_dest1); > > must have a consistent view of what a and b are. So old_val and new_val > cannot at this point reference "newer" values of a and b (as set by previous > instructions in the sequence). AIUI: Sigh. ifcvt seems to pervasively adjust arguments, then you have to figure out which one is the right one for any given context. I was driving me nuts a couple weeks ago when I was looking at the condzero work. It's part of why I set everything down at the time. I ran into it in the VRULL code, Ventana's hacks on top of the VRULL code and in the ESWIN code, got frustrated and decided to look at something else for a bit (which has led to its own little rathole). > > The same cond, new_val and old_val are used in: > > seq2 = try_emit_cmove_seq (if_info, temp, cond, > new_val, old_val, need_cmov, > &cost2, &temp_dest2, cc_cmp, rev_cc_cmp); > > So won't any use of a and b in seq2 to be from cond, rather than old_val > and new_val? If so, shouldn't we set read_comparison for any use of a > and b, rather than skipping IF_THEN_ELSE? Seems like it to me, yes. > > Using seq_cost seems like the best way of costing things. And I agree > that it's worth trying to avoid costing (and generating) redundant > instructions. I think there's general agreement on seq_cost. I wonder if we should look to split that out on its own, then figure out what to do with the bigger issues in this space. Jeff