From: Jeff Law <jeffreyalaw@gmail.com>
To: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] Add new target hook: simplify_modecc_const.
Date: Fri, 14 Oct 2022 14:31:23 -0600 [thread overview]
Message-ID: <367ff707-58ab-7bb1-79d0-1ad2d4fb9f1a@gmail.com> (raw)
In-Reply-To: <CAMe9rOrub2edt80Q9jRZZRTJKs4kPoF_WsLDT0S3jmLOzKBWvw@mail.gmail.com>
On 10/10/22 09:50, H.J. Lu via Gcc-patches wrote:
> On Thu, Jul 28, 2022 at 5:40 AM Richard Sandiford via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> Seems this thread has become a bit heated, so I'll try to proceed
>> with caution :-)
>>
>> In the below, I'll use "X-mode const_int" to mean "a const_int that
>> is known from context to represent an X-mode value". Of course,
>> the const_int itself always stores VOIDmode.
>>
>> "Roger Sayle" <roger@nextmovesoftware.com> writes:
>>> 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.
>> FWIW, I agree this distinction is important, with the proviso (which
>> I think you were also adding) that the code never loses track of what
>> mode an rtx operand (stored in a variable) actually has/is being
>> interpreted to have.
>>
>> In other words, the reason (zero_extend (const_int N)) is invalid is
>> not that constant integers can't be extended in principle (of course
>> they can). It's invalid because we've lost track of how many bits
>> that N actually has. That problem doesn't apply in contexts where
>> the operation is described using individual variables (rather than
>> a single rtx) *provided that* one of those variables says what mode
>> any potential const_ints actually represent.
>>
>>> 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.
>> And the mode argument to simplify_const_relational_operation specifies
>> the mode of the operands, not the mode of the result. I.e. it specifies
>> the modes of op0 and op1 rather than the mode that would be attached to
>> the code in "(code:mode ...)" if an rtx were created with these parameters.
>>
>> That confused me when I saw the patch initially. Elsewhere in the
>> file "mode" tends to be the mode of the result, in cases where the
>> mode of the result can be different from the modes of the operands,
>> so using it for the mode of the operands seems a bit confusing
>> (not your fault of course).
>>
>> I still struggle with the idea of having CC-mode const_ints though
>> (using the meaning of "CC-mode const_ints" above). I realise
>> (compare (...) (const_int 0)) has been the norm "for ever", but here
>> it feels like we're also blessing non-zero CC-mode const_ints.
>> That raises the question of how many significant bits a CC-mode
>> const_int actually has. Currently:
>>
>> ... For historical reasons,
>> the size of a CC mode is four units.
>>
>> But treating CC-mode const_ints as having 32 significant bits is surely
>> the wrong thing to do.
>>
>> So if we want to add more interpretation around CC modes, I think we
>> should first clean up the representation to make the set of valid values
>> more explicit. (Preferably without reusing const_int for constant values,
>> but that's probably a losing battle :-))
>>
>> Thanks,
>> Richard
> Here is a testcase to show that combine generates
>
> (set (reg:CCC 17 flags)
> (ltu:SI (const_int 1 [1])
> (const_int 0 [0])))
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107172
>
> This new target hook handles it properly
ANd does it work if you reject MODE_CC comparisons with two constants in
simplify_const_relational_operation?
I suspect it will work, but generate suboptimal code.
jeff
next prev parent reply other threads:[~2022-10-14 20:31 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-07 21:08 [PATCH] Be careful with MODE_CC in simplify_const_relational_operation Roger Sayle
2022-07-07 22:38 ` Segher Boessenkool
2022-07-26 12:13 ` [PATCH] Add new target hook: simplify_modecc_const Roger Sayle
2022-07-26 17:44 ` Segher Boessenkool
2022-07-26 21:04 ` Roger Sayle
2022-07-26 22:11 ` Segher Boessenkool
2022-07-27 7:51 ` Roger Sayle
2022-07-27 18:37 ` Segher Boessenkool
2022-07-28 12:39 ` Richard Sandiford
2022-10-10 15:50 ` H.J. Lu
2022-10-14 20:31 ` Jeff Law [this message]
2022-10-14 21:05 ` H.J. Lu
2022-10-14 20:26 ` Jeff Law
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=367ff707-58ab-7bb1-79d0-1ad2d4fb9f1a@gmail.com \
--to=jeffreyalaw@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).