> On 7/7/26 3:23 AM, Jose E. Marchesi wrote:
>> I used the following program, which is derived from the
>> divmod-libcall-1.c and divmod-libcall-2.c testcases:
>>
>> int
>> foodiv (unsigned int len) // cpu=v3 NO libcall
>> {
>> return ((unsigned long)len) * 234 / 5;
>> }
>
> [snip modulo example] Focusing on DIV for now.
>
>> long // cpu=v3 ALWAYS LIBCALL
>> bardiv (long x)
>> {
>> return x * 234 / 5;
>> }
>
> [snip modulo again]
> Also worth noting that this variant is as modified by my patch. The
> trunk version is
>
> int
> bardiv2 (unsigned int len)
> {
> return ((long)len) * 234 / 5;
> }
>
>
> which is arguably ambiguous as the dividend coming into function is
> provably unsigned, so compiler can just emit the baseline unsigned
> divide insn, eliding the libcall. Making it genuinely signed long
> would always generate the libcall. Anyhow this is not important but
> wanted to clarify
>
>
>> With your patch as-is, which assigns cost 1 to imm32 operands in
>> mult/div/udiv/mod/umod instructions, i.e.
>>
>> case CONST_INT:
>> {
>> HOST_WIDE_INT val = INTVAL (x);
>> /* BPF ALU instructions take a signed 32-bit immediate operand. */
>> bool imm32 = (val == (HOST_WIDE_INT) (int32_t) val);
>>
>> switch (outer_code)
>> {
>> /* Do not report a free constant when it is the operand of a
>> multiply, divide or modulo. A zero-cost constant misleads
>> synth_mult () and expand_divmod () into implementing the
>> operation as a sequence of shifts/adds (or a magic-number
>> multiply) instead of BPF's native single-instruction mul/div,
>> which is cheaper here. */
>> case MULT:
>> case DIV:
>> case UDIV:
>> case MOD:
>> case UMOD:
>> *total = COSTS_N_INSNS (1);
>> break;
>>
>> default:
>> /* An immediate operand is free; a wider constant needs an
>> extra LD_IMM64. */
>> *total = imm32 ? 0 : COSTS_N_INSNS (1);
>> }
>>
>> return true;
>> }
>>
>> I get:
>>
>>
>> -O2 -mcpu=v3
>>
>> foodiv:
>> w0 = w1
>> r0 *= 234
>> r0 /= 5
>> exit
>>
>> bardiv:
>> r2 = 5
>> r1 *= 234
>> call __divdi3
>> exit
>>
>>
>> -O2 -mcpu=v4
>>
>> foodiv:
>> w0 = w1
>> r0 *= 234
>> r0 /= 5
>> exit
>>
>> bardiv:
>> r0 = r1
>> r0 *= 234
>> r0 s/= 5
>> exit
>>
>> Which is as expected.
>
> Right.
>
>> Then, modifying your patch so it assigns cost 0 to imm32 immediates also
>> in div, mod and mul instructions, i.e.
>>
>> case CONST_INT:
>> {
>> HOST_WIDE_INT val = INTVAL (x);
>> /* BPF ALU instructions take a signed 32-bit immediate operand. */
>> bool imm32 = (val == (HOST_WIDE_INT) (int32_t) val);
>>
>> /* An immediate operand is free; a wider constant needs an
>> extra LD_IMM64. */
>> *total = imm32 ? 0 : COSTS_N_INSNS (1);
>> return true;
>> }
>>
>> I get:
>>
>> -O2 -mcpu=v3
>>
>> foodiv:
>> w0 = w1
>> r0 *= 234
>> r0 /= 5
>> exit
>>
>> bardiv:
>> r2 = 5
>> r1 *= 234
>> call __divdi3
>> exit
>>
>> -O2 -mcpu=v4
>>
>> foodiv:
>> w0 = w1
>> r0 *= 234
>> r0 /= 5
>> exit
>>
>> bardiv:
>> r0 = r1
>> r0 *= 234
>> r0 s/= 5
>> exit
>>
>> Wich is also as expected.
>
> Doh' you are right. I swear there was something at the time that
> needed this which I can't seem to reproduce now. So indeed that hunk
> is not needed.
>
> The third hunk for adjusting the cost of MPY/DIV/MOD themselves to be
> 1 (vs. the default 5, 7...) is necessary.
> W/o those we do get regressions on the div mode*, memset-4 (for mult32)
> In fact it is needed to fix the following trivial multiply
>
> unsigned mul7 (unsigned x) { return x * 7; }
>
> On trunk it is synthesized using shift 3 + minus, w/ that change it
> uses MPY.
Yes that makes sense to me. The cost of a shift-by-constant followed by
a subtract-by-constant would be something like 2.
> So we can break this up into 2 seperate changes: the MPY/DIV cost adj
> is first / independent fix, followed by cost model for const_int
> (although the MPY/DIV is also needed to not regress 2nd)
>
> BTW I just noted that in the other costing hook TARGET_INSN_COST /
> bpf_insn_cost, is returning cost 1 as default, whereas targets
> typically use pattern_cost () for default. Since we are on the topic
> I'll fix that too but as an additional patch unless you prefer them in
> one. Right now its just preventive.
>
> If we agree, I'll spin a v2 along those lines.
Two patches for bpf_rtx_costs would work, thanks.
> Thanks for keeping me honest and apologies for the confusion.
No need to apologize, really. As everyone knows I get confused all the
time. Much on the contrary, thanks to _you_ for your help.
