[DTrace-devel] Reviewed: Do not hard-code the temp register in aggregation functions

[Eugene Loh]

On 11/25/2020 09:28 PM, Kris Van Hees wrote:

> On Wed, Nov 25, 2020 at 05:34:48PM -0800, Eugene Loh wrote:
>> On 11/20/2020 02:15 PM, kris.van.hees at oracle.com wrote:
>>> [Commit 15efba57c50081ecba70bc236cb25f619c3f1c48]
>>>> Do not hard-code the temp register in aggregation functions
>>>>
>>>> A few aggregation functions were hard-coding the temp register
>>>> to BPF_REG_0.  Let dt_regset_alloc() pick the register.
>>> Actually, for code that needs to use a short-term temporary register, %r0
>>> is an ideal choice as long as you do not make any function calls or call
>>> dt_cg_node() to generate code for a subtree.  So, using it as a scratch
>>> register is perfectly fine, and often preferred because it reduces the
>>> pressure on the dynamically allocated registers.
>> I'm confused.  Are you saying you're against this patch? Presumably, in
>> these functions, we do not know which registers are currently in use.
>> Presumably, that is why we explicitly dt_regset_xalloc(BPF_REG_0) here.
>> (In contrast, there are places we assume we know what the register usage
>> is, and we hardcode register usage without regset alloc/free at all.)
>> So, it seems to me the code would be more robust if we ask
>> regset_alloc() to give us a free register rather than specifying which
>> register to give us.  That induces no register pressure since it's going
>> to give us a register that's free.  Our reasoning about which registers
>> are free could be faulty;  in contrast, regset knows which registers are
>> free.
> Yes, I am against this patch.  While we indeed have dynamic allocation of
> registers using regset alloc, there are cases where we know that %r0 can be
> use without conflict.  These are cases where we know %r0 will not be in use
> prior to the code getting called,

Thanks for the explanations, but -- regardless of the fate of this 
particular patch -- I'd like to understand these issues.

How is it that we know that %r0 will not be in use prior to the code 
getting called?

In any case, even if it is possible to know that %r0 is not in use 
(depending on how smart the developer is, with you being smarter than 
me), what is the problem with just using any free register? That just 
seems like a safer, more robust programming practice, even if a smarter 
developer like you is able to get by without such a safety net.  (The 
you/me thing is just that *I* don't think I'll always assess correctly 
whether %r0 is okay in some situation.  I don't even understand this 
situation.)

> and we do not call any other codegen
> functions (which means that nothing else could possibly use %r0 as well).
> While we could use a dynamically allocated register in those cases, it is
> perfectly fine to use %r0 when we know it is safe.  It is an ideal scratch
> register for temporary values, and that is a common use in BPF code actually.
>
> We register its use simply because that provides a concenient check that
> indeed nothing should have reserved it prior to its use here,

I don't think it checks that %r0 is not reserved.  If %r0 is in use and 
we specifically ask for it, then %r0 is spilled and we're allowed to use 
it.  So, if a developer (presumably someone like me rather than someone 
like you) makes a mistake and explicitly asks for %r0 when it's already 
in use, we get unnecessary spills and fills.  In contrast, had we asked 
for "a free register," the unnecessary spills are avoided.

> and nothing
> should reserve it until we free it.  It also ensures that it shows up in
> reg usage tracing output.
>
> ANd yes, there are cases where we hardcode register usage without using
> alloc/free at all, and as I have communicated already, those need to be
> addressed soon to rectify that oversight.