Re: [PATCH 0/2] RISC-V: Define not broken prefetch builtins

[Jeff Law <jeffreyalaw@gmail.com>]

On 10/22/23 21:55, Tsukasa OI wrote:

>> What I still don't understand is why we're dealing with a decomposed
>> address in the builtin, define_expand and/or define_insn.

> 
> Sorry, I misunderstood your intent (quite badly) possibly because I was
> not familiar with the concept of "predicates" in GCC.
OK.  So you might want to read the machine description part of the GCC 
manual.  It describes operand predicates, operand constraints, insn 
conditions, the difference between define_insn vs define_expand and much 
more.


> 
> On 2023/08/29 6:20, Jeff Law wrote:
>> What I would suggest is making a new predicate that accepts either a
>> register or a register+offset where the offset fits in a signed 12 bit
>> immediate.  Use that for operand 0's predicate and I think this will
>> "just work" and cover all the cases supported by the prefetch.i instruction.
> 
> I misunderstood that as "just" adding the offset field to the
> instructions and that's the reason I veered off the path so much.  So
> instead, I'll answer your original question.
> 
> register+offset seems a problem for prefetch instructions because signed
> 12 bit immediate values need to be also a multiple of 32.  There's no
> proper relocation type for this kind and I considered we have "very"
> limited cases where making such predicate (as you suggested) will
> *efficiently* work.
> 
> My opinion is, if we need very fine-grained control with prefetch
> instructions, we'd better to use inline assembly.
> 
> I'll continue testing the possibilities of register+offset predicate
> (including whether it works efficiently) and I'll temporarily withdraw
> new built-in functions to focus on major issues before GCC 14:
> 
> 1.  Remove completely broken __builtin_riscv_zicbop_prefetch_i and
> 2.  Fix an ICE when __builtin_prefetch is used with some constants.
> 
> I'll submit minimized patches only to fix those issues.  They will not
> contain "register+offset" you suggested because of the difficulties
> above but should be sufficient to fix imminent issues.
We should be able to describe this need quite easily.

Each operand has a predicate which the compiler tests to see if a 
particular RTL expression matches.  Some are very generic like 
"register_operand".  Others are target specific.  If you look in 
predicates.md you'll see a list of the predicates already defined for 
risc-v.  I'm pretty sure none of them will work for this case, but we 
can add a new one easily.

The operand in question is going to be a MEM with restrictions on its 
addressing mode.  Either REG or REG + aligned offset.

(define_predicate "prefetch_memory_operand"
   (match_code "mem")
{
   op = XEXP (op, 0);
   return (REG_P (op)
           || (GET_CODE (op) == PLUS
               && REG_P (XEXP (op, 0))
               && CONST_INT_P (XEXP (op, 1))
               && (INTVAL (XEXP (op, 1)) % 32) == 0);
}

[ Note that we did not declare "op".  It's provided by the generator and 
corresponds to the operand we're testing. ]

So you're going to want a define_expand for the basic prefetch

(define_expand "riscv_prefetch_r_<mode>"
   [(unspec_volatile:X [(match_operand:X 0 "memory_operand")]
                        UNSPEC_PREFETCH_R)]
   "TARGET_ZICBOP"
{
   if (!prefetch_memory_operand (Pmode, operands[0])
     XEXP (operands[0], 0) = force_reg (Pmode, XEXP (operands[0], 0);
}

The thing to know about a define_expand is that it's sole purpose is for 
RTL generation purposes.   We can use it as a place to adjust operands 
(as is done in this case), or emit additional RTL such as we do for 
SImode max on rv64 where we have to extend the incoming operands.

In our case we see if the memory address matches 
prefetch_memory_operand, and if not it'll force that address into a new 
register to create a (mem (reg)) object.



(define_insn "*riscv_prefetch_r_<mode>"
   [(unspec_volatile:X [(match_operand:X 0 "prefetch_memory_operand")]
                        UNSPEC_PREFETCH_R)]
   "TARGET_ZICBOP"
   "prefetch.r\t%0"
   [(set_attr "type" "cbo")])

The define_insn construct maps an RTL template to assembly code with 
provisions for testing operands and such.

Anyway, hopefully that makes things clearer.


Jeff