Re: [PATCH 2/2] rs6000: Refine integer comparison handlings in rs6000_emit_vector_compare

[Segher Boessenkool <segher@kernel.crashing.org>]

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