From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 1A631384F6D2 for ; Fri, 18 Nov 2022 15:19:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1A631384F6D2 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 2AIFIuAY017094; Fri, 18 Nov 2022 09:18:56 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 2AIFIuO9017093; Fri, 18 Nov 2022 09:18:56 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 18 Nov 2022 09:18:55 -0600 From: Segher Boessenkool To: "Kewen.Lin" Cc: GCC Patches , David Edelsohn , Peter Bergner , Michael Meissner Subject: Re: [PATCH 2/2] rs6000: Refine integer comparison handlings in rs6000_emit_vector_compare Message-ID: <20221118151855.GB25951@gate.crashing.org> References: <247bf71b-e0ab-7cf7-098b-a106a0764301@linux.ibm.com> <20221116185827.GX25951@gate.crashing.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP 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: Hi! On Thu, Nov 17, 2022 at 03:52:26PM +0800, Kewen.Lin wrote: > on 2022/11/17 02:58, Segher Boessenkool wrote: > > On Wed, Nov 16, 2022 at 02:51:04PM +0800, Kewen.Lin wrote: > >> /* In vector.md, we support all kinds of vector float point > >> comparison operators in a comparison rtl pattern, we can > >> just emit the comparison rtx insn directly here. Besides, > >> we should have a centralized place to handle the possibility > >> - of raising invalid exception. */ > >> - if (GET_MODE_CLASS (dmode) == MODE_VECTOR_FLOAT) > >> + of raising invalid exception. Also emit directly for vector > >> + integer comparison operators EQ/GT/GTU. */ > >> + if (GET_MODE_CLASS (dmode) == MODE_VECTOR_FLOAT > >> + || rcode == EQ > >> + || rcode == GT > >> + || rcode == GTU) > > > > The comment still says it handles FP only. That would be best to keep > > imo: add a separate block of code to handle the integer stuff you want > > to add. You get the same or better generated code, the compiler is > > smart enough. Code is for the user to read, and C is not a portable > > assembler language. > > OK, I'll make two blocks for FP and integer respectively. I struggled > a bit updating this hunk with comments for integer comparison > consideration, someone could argue that both can share the same handling > if updating the condition. The golden rule of programming: if something is hard to explain, you probably overcomplicated it. Sometimes more code is much easier to read, too. > > This whole series needs to be factored better, it does way too many > > things, and only marginally related things, at every step. Or I don't > > see it anyway :-) > > OK, I was thinking patch 1/2 is to unify the current vector float > comparison handlings, patch 2/2 is to refine the remaining handlings > for vector integer comparison. I'm pleased to factor it better, any > suggestions on concrete code is highly appreciated. :) Often it helps to start with a patch (or patches) that only restructures existing code, without changing what it does, so that the patch that does change anything material is smaller and easier to read and review. The (perhaps big) patch that doesn't change anything is easy to review as well then, simple because it *says* it does not change anything, and reviewing it boils down to verifying that is true. Segher