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

["Roger Sayle" <roger@nextmovesoftware.com>]

Hi Segher,
It's very important to distinguish the invariants that exist for the RTL
data structures as held in memory (rtx), vs. the use of "enum rtx_code"s,
"machine_mode"s and operands in the various processing functions
of the middle-end.

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.  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.  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.

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

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.

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.

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.

Thanks in advance,
Roger
--

> -----Original Message-----
> From: Segher Boessenkool <segher@kernel.crashing.org>
> Sent: 26 July 2022 18:45
> To: Roger Sayle <roger@nextmovesoftware.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] Add new target hook: simplify_modecc_const.
> 
> Hi!
> 
> On Tue, Jul 26, 2022 at 01:13:02PM +0100, Roger Sayle wrote:
> > This patch is a major revision of the patch I originally proposed here:
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598040.html
> >
> > The primary motivation of this patch is to avoid incorrect
> > optimization of MODE_CC comparisons in
> > simplify_const_relational_operation when/if a backend represents the
> > (known) contents of a MODE_CC register using a CONST_INT.  In such
> > cases, the RTL optimizers don't know the semantics of this integer
> > value, so shouldn't change anything (i.e. should return NULL_RTX from
> simplify_const_relational_operation).
> 
> This is invalid RTL.  What would  (set (reg:CC) (const_int 0))  mean, for
example?
> If this was valid it would make most existing code using CC modes do
essentially
> random things :-(
> 
> The documentation (in tm.texi, "Condition Code") says
>   Alternatively, you can use @code{BImode} if the comparison operator is
>   specified already in the compare instruction.  In this case, you are not
>   interested in most macros in this section.
> 
> > The worked example provided with this patch is to allow the i386
> > backend to explicitly model the carry flag (MODE_CCC) using 1 to
> > indicate that the carry flag is set, and 0 to indicate the carry flag
> > is clear.  This allows the instructions stc (set carry flag), clc
> > (clear carry flag) and cmc (complement carry flag) to be represented in
RTL.
> 
> Hrm, I wonder how other targets do this.
> 
> On Power we have a separate hard register for the carry flag of course (it
is a
> separate bit in the hardware as well, XER[CA]).
> 
> On Arm there is arm_carry_operation (as well as arm_borrow_operation).
> 
> Aarch64 directly uses
> (define_expand "add<mode>3_carryin"
>   [(set (match_operand:GPI 0 "register_operand")
>         (plus:GPI
>           (plus:GPI
>             (ltu:GPI (reg:CC_C CC_REGNUM) (const_int 0))
>             (match_operand:GPI 1 "aarch64_reg_or_zero"))
>           (match_operand:GPI 2 "aarch64_reg_or_zero")))]
>    ""
>    ""
> )
> (CC_Cmode means only the C bit is validly set).
> 
> s390 does similar.  sparc does similar.
> 
> > However an even better example would be the rs6000 backend, where this
> > patch/target hook would allow improved modelling of the condition
> > register CR.  The powerpc's comparison instructions set fields/bits in
> > the CR register [where bit 0 indicates less than, bit 1 greater than,
> > bit 2 equal to and bit3 overflow]
> 
> There are eight condition register fields which can be used
interchangeably
> (some insns only write to CR0, CR1, or CR6).  The meaning of the four bits
in a
> field depends on the instruction that set them.  For integer comparisons
bit 3
> does not mean anything to do with a
> comparison: instead, it is a copy of the XER[SO] bit ("summary overflow").
The
> rs6000 backend does not currently model this (we do not model the overflow
> instructions at all!)
> 
> > analogous to x86's flags register [containing bits for carry, zero,
> > overflow, parity etc.].  These fields can be manipulated directly
> > using crset (aka creqv) and crclr (aka crxor) instructions
> 
> crand, crnand, cror, crxor, crnor, creqv, crandc, crorc insns, or the
extended
> mnemonics crmove, crclr, crnot, crset, yes.  All these for setting single
bits;
> there also is mcrf to copy all four bits of a CR field to another.
> 
> > and even transferred from general purpose registers using mtcr.
> > However, without a patch like this, it's impossible to safely
> > model/represent these instructions in rs6000.md.
> 
> And yet we do.  See for example @cceq_rev_compare_<mode> which
> implements crnot.
> 
> > +  /* Handle MODE_CC comparisons that have been simplified to
> > +     constants.  */
> > +  if (GET_MODE_CLASS (mode) == MODE_CC
> > +      && op1 == const0_rtx
> > +      && CONST_INT_P (op0))
> > +    return targetm.simplify_modecc_const (mode, (int)code, op0);
> 
> Comparing two integer constants is invalid RTL *in all contexts*.  The
items
> compared do not have a mode!  From rtl.texi:
>   A @code{compare} specifying two @code{VOIDmode} constants is not valid
>   since there is no way to know in what mode the comparison is to be
>   performed; the comparison must either be folded during the compilation
>   or the first operand must be loaded into a register while its mode is
>   still known.
> 
> 
> Segher