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

[Kris Van Hees]

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, 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, 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.