Thank you again for your helpful comments.
I will implement the formatting changes you mentioned.
I agree that a description like:
;; Transform (X & C1) + C2 into (X | ~C1) - (-C2 | ~C1)
;; Where C1 is not a LUI operand, but ~C1 is a LUI operand
is more fitting since we are catching a plus expression.
Your proposed condition also makes more sense than the current one.
LUI_OPERAND (INTVAL (operands[2]) + 1) made sense when starting out,
but I never got around to changing to LUI_OPERAND (~INTVAL (operands[2]).
I did not know about the costs of constant synthesis.
I thought that the values of those constants are known at compile time, and
therefore their creation would not have an impact on performance.
I will definitely take a look at riscv_const_insns and evaluate the synthesis
complexity.
Best regards,
Oliver
________________________________
From: Jeff Law <jeffreya...@gmail.com>
Sent: Sunday, December 15, 2024 1:19 AM
To: Oliver Kozul <oliver.ko...@rt-rk.com>; gcc-patches@gcc.gnu.org
<gcc-patches@gcc.gnu.org>
Cc: Dusan Stojkovic <dusan.stojko...@rt-rk.com>; Mile Davidovic
<mile.davido...@rt-rk.com>; l...@gcc.gnu.org <l...@gcc.gnu.org>
Subject: Re: [PATCH] RISC-V: optimization on checking certain bits set ((x &
mask) == val)
On 12/6/24 6:12 AM, Oliver Kozul wrote:
> The patch optimizes code generation for comparisons of the form
> X & C1 == C2 by converting them to (X | ~C1) == (C2 | ~C1).
> C1 is a constant that requires li and addi to be loaded,
> while ~C1 requires a single lui instruction.
>
> 2024-12-06 Oliver Kozul <oliver.ko...@rt-rk.com>
>
> PR target/114087
>
> gcc/ChangeLog:
>
> * config/riscv/riscv.md (*lui_constraint<ANYI:mode>_and_to_or):
> New pattern.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/pr114087-1.c: New test.
A few comments:
>
> +(define_insn_and_split "*lui_constraint<ANYI:mode>_and_to_or"
> + [(set (match_operand:ANYI 0 "register_operand" "=r")
> + (plus:ANYI (and:ANYI (match_operand:ANYI 1 "register_operand" "r")
> + (match_operand 2 "const_int_operand"))
> + (match_operand 3 "const_int_operand")))
> + (clobber (match_scratch:X 4 "=&r"))]
Similar formatting comment as I had on the other patch. Easily fixed.
Just for giggles I asked chatgpt to format it and it came up with:
> (define_insn_and_split "*lui_constraint<ANYI:mode>_and_to_or"
> [(set (match_operand:ANYI 0 "register_operand" "=r")
> (plus:ANYI (and:ANYI (match_operand:ANYI 1 "register_operand" "r")
> (match_operand 2 "const_int_operand"))
> (match_operand 3 "const_int_operand")))
> (clobber (match_scratch:X 4 "=&r"))]
> "LUI_OPERAND (INTVAL (operands[2]) + 1)
> && (INTVAL (operands[2]) & (-INTVAL (operands[3]))) == (-INTVAL
> (operands[3]))"
> "#"
> "&& reload_completed"
> [(set (match_dup 4) (match_dup 5))
> (set (match_dup 0) (ior:X (match_dup 1) (match_dup 4)))
> (set (match_dup 4) (match_dup 6))
> (set (match_dup 0) (minus:X (match_dup 0) (match_dup 4)))]
> {
> operands[5] = GEN_INT (~INTVAL (operands[2]));
> operands[6] = GEN_INT ((~INTVAL (operands[2])) | (-INTVAL (operands[3])));
> }
> [(set_attr "type" "arith")])
Which looks basically correct.
More importantly we probably want a comment on what this pattern is
actually doing. Perhaps something like:
;; Transform (X & C1) + C2 into (X | ~C1) - (-C2 | ~C1)
;; Where C1 is not a LUI operand, but ~C1 is a LUI operand
Did I get that correct?
> + "LUI_OPERAND (INTVAL (operands[2]) + 1)
So shouldn't this have been LUI_OPERAND (~INTVAL (operands[2]))? I can
kind-of see why you used +1, but given the C fragment in the pattern
uses ~operands[2], let's use the same here for the test if we can.
> + && (INTVAL (operands[2]) & (-INTVAL (operands[3])))
> + == (-INTVAL (operands[3]))"
Does it make sense to verify that operands[3] is a constant that
requires synthesis and that ~INTVAL (operands[2]) | -INTVAL
(operands[3]) is no more expensive to synthesize than operands[3]?
ie, saving the addi on the first constant isn't a win if the second
constant gets more complex to synthesize. The API isn't great, but you
can use riscv_const_insns to find out how many instructions are
necessary to synthesize a constant.
Overall it looks good. Just needs a bit of polishing.
jeff
CONFIDENTIALITY: The contents of this e-mail are confidential and intended only
for the above addressee(s). If you are not the intended recipient, or the
person responsible for delivering it to the intended recipient, copying or
delivering it to anyone else or using it in any unauthorized manner is
prohibited and may be unlawful. If you receive this e-mail by mistake, please
notify the sender and the systems administrator at straym...@rt-rk.com
immediately.
