public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Palmer Dabbelt <palmer@dabbelt.com>
To: gcc-patches@gcc.gnu.org
Cc: sinan.lin@linux.alibaba.com, gcc-patches@gcc.gnu.org
Subject: Re: 回复:[PING] [PATCH RESEND] riscv: improve the cost model for loading a 64bit constant in rv32.
Date: Mon, 28 Nov 2022 11:18:24 -0800 (PST)	[thread overview]
Message-ID: <mhng-b1640937-e124-4916-939f-5a9fc2214f5d@palmer-ri-x1c9a> (raw)
In-Reply-To: <f5423f3a-7e34-3c2d-3b90-7552d9e25f54@gmail.com>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6781 bytes --]

On Mon, 28 Nov 2022 11:15:01 PST (-0800), gcc-patches@gcc.gnu.org wrote:
>
>
> On 11/24/22 00:43, Sinan wrote:
>>> The motivation of this patch is to correct the wrong estimation of
>>>> the number of instructions needed for loading a 64bit constant in
>>>> rv32 in the current cost model(riscv_interger_cost). According to
>>>> the current implementation, if a constant requires more than 3
>>>> instructions(riscv_const_insn and riscv_legitimate_constant_p),
>>>> then the constant will be put into constant pool when expanding
>>>> gimple to rtl(legitimate_constant_p hook and emit_move_insn).
>>>> So the inaccurate cost model leads to the suboptimal codegen
>>>> in rv32 and the wrong estimation part could be corrected through
>>>> this fix.
>>>>
>>>> e.g. the current codegen for loading 0x839290001 in rv32
>>>>
>>>>    lui     a5,%hi(.LC0)
>>>>    lw      a0,%lo(.LC0)(a5)
>>>>    lw      a1,%lo(.LC0+4)(a5)
>>>> .LC0:
>>>>    .word   958988289
>>>>    .word   8
>>>>
>>>> output after this patch
>>>>
>>>>    li a0,958988288
>>>>    addi a0,a0,1
>>>>    li a1,8
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>          * config/riscv/riscv.cc (riscv_build_integer): Handle the case of loading 64bit constant in rv32.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>>          * gcc.target/riscv/rv32-load-64bit-constant.c: New test.
>>>>
>>>> Signed-off-by: Lin Sinan <sinan.lin@linux.alibaba.com>
>>>> ---
>>>>   gcc/config/riscv/riscv.cc                     | 23 +++++++++++
>>>>   .../riscv/rv32-load-64bit-constant.c          | 38 +++++++++++++++++++
>>>>   2 files changed, 61 insertions(+)
>>>>   create mode 100644 gcc/testsuite/gcc.target/riscv/rv32-load-64bit-constant.c
>>>>
>>>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>>>> index 32f9ef9ade9..9dffabdc5e3 100644
>>>> --- a/gcc/config/riscv/riscv.cc
>>>> +++ b/gcc/config/riscv/riscv.cc
>>>> @@ -618,6 +618,29 @@ riscv_build_integer (struct riscv_integer_op *codes, HOST_WIDE_INT value,
>>>>    }
>>>>       }
>>>>
>>>> +  if ((value > INT32_MAX || value < INT32_MIN) && !TARGET_64BIT)
>>>
>>> Nit.   It's common practice to have the TARGET test first in a series of
>>> tests.  It may also be advisable to break this into two lines.
>>> Something like this:
>>>
>>>
>>>   if ((!TARGET_64BIT)
>>>       || value > INT32_MAX || value < INT32_MIN)
>>>
>>>
>>> That's the style most GCC folks are more accustomed to reading.
>>
>> Thanks for the tips and I will change it then.
>>
>>>> +    {
>>>> +      unsigned HOST_WIDE_INT loval = sext_hwi (value, 32);
>>>> +      unsigned HOST_WIDE_INT hival = sext_hwi ((value - loval) >> 32, 32);
>>>> +      struct riscv_integer_op alt_codes[RISCV_MAX_INTEGER_OPS],
>>>> +       hicode[RISCV_MAX_INTEGER_OPS];
>>>> +      int hi_cost, lo_cost;
>>>> +
>>>> +      hi_cost = riscv_build_integer_1 (hicode, hival, mode);
>>>> +      if (hi_cost < cost)
>>>> + {
>>>> +   lo_cost = riscv_build_integer_1 (alt_codes, loval, mode);
>>>> +   if (lo_cost + hi_cost < cost)
>>>
>>> Just so I'm sure.  "cost" here refers strictly to other synthesized
>>> forms? If so, then ISTM that we'd want to generate the new style when
>>> lo_cost + hi_cost < cost OR when lo_cost + hi_cost is less than loading
>>> the constant from memory -- which is almost certainly more than "3"
>>> since the sequence from memory will be at least 3 instructions, two of
>>> which will hit memory.
>>>
>>>
>>> Jeff
>>>
>>
>> Yes, almost right. The basic idea of this patch is to improve the cost
>> calculation for loading 64bit constant in rv32, instead of adding a new
>> way to load constant.
>>
>> gcc now loads 0x739290001LL in rv32gc with three instructions,
>>          li      a0,958988288
>>          addi    a0,a0,1
>>          li      a1,7
>> However, when it loads 0x839290001LL, the output assembly becomes
>>          lui     a5,%hi(.LC0)
>>          lw      a0,%lo(.LC0)(a5)
>>          lw      a1,%lo(.LC0+4)(a5)
>>      .LC0:
>>          .word   958988289
>>          .word   8
>> The cost calculation is inaccurate in such cases, since loading these
>> two constants should have no difference in rv32 (just change `li a1,7`
>> to `li a1,8` to load the hi part). This patch will take these cases
>> into consideration.
>>
> I think I see better what's going on.  This really isn't about the
> constant pool costing.  It's about another way to break down the
> constant into components.
>
> riscv_build_integer_1, for the cases we're looking at breaks down the
> constant so that high + low will give the final result.  It costs the
> high and low parts separately, then sums their cost + 1 for the addition
> step.
>
> Your patch adds another method that is specific to rv32 and takes
> advantage of register pairs.   You break the constant down into 32bit
> high and low chunks, where each chunk will go into a different 32 bit
> register.  You just then need to sum the cost of loading each chunk.
>
> For the constants in question, your new method will result in a smaller
> cost than the current method.   That's really the point of
> riscv_build_integer -- find the sequence and cost of creation.  We later
> use that information to determine if we should use that sequence or a
> constant pool.
>
> Palmer raised an issue on the tests with a request to not include the
> arch/abi specification.  But I think you addressed that in a later
> comment.  Specifically for rv64 we end up with another instruction,
> which would cause some constants to be considered cheaper as constant
> pool entries rather than inline sequences.
>
> Palmer is right in this seems like it ought to be generic, particularly
> breaking things down on word boundaries.  But I don't think adding that
> infrastructure should hold this patch up.  Reality is not much is
> happening with 32bit (or smaller) architectures and little is happening
> with 128bit integer types.  So there's not much motivation to fix this
> stuff more generically right now.

Seems reasonable to me, we can always promote it to something generic 
later if some other port wants something similar.

  reply	other threads:[~2022-11-28 19:18 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-10 14:37 Lin Sinan
2022-11-17  7:32 ` [PING] " Lin Sinan
2022-11-22 22:23 ` Jeff Law
     [not found]   ` <e026c2e0-04ec-4477-bd91-441a6f160251.sinan.lin@linux.alibaba.com>
2022-11-28 19:15     ` 回复:[PING] " Jeff Law
2022-11-28 19:18       ` Palmer Dabbelt [this message]
2022-11-28 19:44 ` 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=mhng-b1640937-e124-4916-939f-5a9fc2214f5d@palmer-ri-x1c9a \
    --to=palmer@dabbelt.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=sinan.lin@linux.alibaba.com \
    /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).