Re: [PATCH] Add new target hook: simplify_modecc_const.

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

Hi!

On Tue, Jul 26, 2022 at 10:04:45PM +0100, Roger Sayle wrote:
> It's very important to distinguish the invariants that exist for the RTL
> data structures as held in memory (rtx),

"In memory"?  What does that mean here?  RTX are just RTL expressions,
nothing more, nothing less.

> vs. the use of "enum rtx_code"s,
> "machine_mode"s and operands in the various processing functions
> of the middle-end.

Of course.

> Yes, it's very true that RTL integer constants don't specify a mode
> (are VOIDmode), so therefore operations like ZERO_EXTEND or EQ
> don't make sense with all constant operands.

ZERO_EXTEND makes sense for all non-negative operands and no negative
operands.  Anything with some integer mode (so not VOIDmode!) can be
the first argument to EQ, at least structurally.

> This is (one reason)
> why constant-only operands are disallowed from RTL (data structures),
> and why in APIs that perform/simplify these operations, the original
> operand mode (of the const_int(s)) must be/is always passed as a
> parameter.
> 
> Hence, for say simplify_const_binary_operation, op0 and op1 can
> both be const_int, as the mode argument specifies the mode of the
> "code" operation. Likewise, in simplify_relational_operation, both
> op0 and op1 may be CONST_INT as "cmp_mode" explicitly specifies
> the mode that the operation is performed in and "mode" specifies
> the mode of the result.

> Your comment that "comparing two integer constants is invalid
> RTL *in all contexts*" is a serious misunderstanding of what's
> going on.

Not at all.  I showed the quote from the documentation: it is always
invalid to have two VOIDmode arguments to COMPARE.

> At no point is a RTL rtx node ever allocated with two
> integer constant operands.  RTL simplification is for hypothetical
> "what if" transformations (just like try_combine calls recog with
> RTL that may not be real instructions), and these simplifcations
> are even sometimes required to preserve the documented RTL
> invariants.  Comparisons of two integers must be simplified to
> true/false precisely to ensure that they never appear in an actual
> COMPARE node.

As the function documentation clearly states, two VOIDmode args (and
MODE that as well) is a special case for infinite precision arithmetic.

> I worry this fundamental misunderstanding is the same issue that
> has been holding up understanding/approving a previous patch:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-September/578848.html

https://patchwork.ozlabs.org/project/gcc/patch/001401d7a2a5$5bf07db0$13d17910$@nextmovesoftware.com/

Let's not discuss this in this thread though.

> For a related bug, consider PR rtl-optimization/67382, that's assigned
> to you in bugzilla.  In this case, the RTL optimizers know that both
> operands to a COMPARE are integer constants (both -2), yet the
> compiler still performs a run-time comparison and conditional jump:
> 
>         movl    $-2, %eax
>         movl    %eax, 12(%rsp)
>         cmpl    $-2, %eax
>         je      .L1
> 
> Failing to optimize/consider a comparison between two integer
> constants *in any context* just leads to poor code.

If combine would ever generate invalid RTL, the resulting insn does not
pass recog(), making the combine attempt fail.  This is not the way.

The simplifier (part of combine) has a function to actually simplify
tautologies and contradictions, the simplify_const_relational_operation
function you edited here.

> Hopefully, this clears up that the documented constraints on RTL rtx
> aren't exactly the same as the constraints on the use of rtx_codes in
> simplify-rtx's functional APIs.  So simplify_subreg really gets called
> on operands that are neither REG nor MEM, as this is unrelated to
> what the documentation of the SUBREG rtx specifies.

Thank you for telling the maintainer of combine the basics of what all
of this does!  I hadn't noticed any of that before.

> If you don't believe that op0 and op1 can ever both be const_int
> in this function, perhaps consider it harmless dead code and humor
> me.

They can be, as clearly documented (and obvious from the code), but you
can not ever have that in the RTL stream, which is needed for your patch
to do anything.

I consider it harmful, and not dead.  Sorry.


Do you have comments on the rest?


Segher