[DTrace-devel] [PATCH 03/14] Promote associative integer arrays to 64 bits when loaded

[Kris Van Hees]

On Thu, May 04, 2023 at 03:04:11PM -0400, Eugene Loh via DTrace-devel wrote:
> On 5/4/23 13:10, Eugene Loh wrote:
> 
> > On 5/3/23 23:54, Kris Van Hees wrote:
> > 
> > > On Mon, May 01, 2023 at 11:47:11PM -0400, eugene.loh--- via
> > > DTrace-devel wrote:
> > > > From: Eugene Loh <eugene.loh at oracle.com>
> > > > 
> > > > In commit 7e9ce1eee475 ("Promote integers to 64 bits when loaded"),
> > > > integers were promoted to 64 bits when loaded.  Associative arrays
> > > > were on a different code path.
> > > > 
> > > > Add the promotion to the associative-array code path and add tests
> > > > for different variable types.  Also, do some minor refactoring in
> > > > dt_cg_assoc_op() in anticipation of subsequent refactoring; this
> > > > prep work is done here to improve readability of subsequent patches.
> > > OK on the addition of the promotion.
> > > 
> > > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > > @@ -3988,7 +3988,7 @@ dt_cg_asgn_op(dt_node_t *dnp, dt_irlist_t
> > > > *dlp, dt_regset_t *drp)
> > > >   }
> > > >     static void
> > > > -dt_cg_assoc_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> > > > +dt_cg_assoc_op(dt_node_t *dst, dt_irlist_t *dlp, dt_regset_t *drp)
> > > I do not really agree with this change (and the resulting changes
> > > below)...
> > > This is a function that generates code for a node, and the
> > > convention in the
> > > code generator is that dnp is used for that variable.  The change to
> > > 'dst'
> > > seems to imply that this node is the destination of some operation,
> > > but then
> > > one would expect there to be a source as well, and that is clearly
> > > not the
> > > case.
> 
> Actually, dt_cg_assoc_op() has only one call site, and that's also where the
> only dt_cg_load_var() call site is.  And dt_cg_load_var() uses "dst" rather
> than "dnp" -- that is, "dst" is being used for gvars, lvars, and tvars.  So
> using "dst" also for assoc is not much of a stretch.  In particular, the
> roadmap is for dt_cg_assoc_op() to be swallowed up by dt_cg_load_var(),
> which is the whole motivation for this patch.  Even the "promote" change is
> so motivated:  the promote bug was discovered by comparing dt_cg_assoc_op()
> to dt_cg_load_var().

Yes, but I have to admit that I overlooked that you used 'dst' as argument
name for dt_cg_load_var() when you introduced that function.  That really
ought to be 'dnp' also because (again) it seems odd to have a 'dst' without
a 'src' and these functions really are generating code for a node rather
than copying or moving something from one node to another.

> Nevertheless, here is a different suggestion.  How about I remove the
> dnp-to-dst renaming from this patch -- making this patch exclusively about
> the promote bug -- and leave the renaming until the later patch ("Inline
> assoc_op() into load_var()"), where the consistent dnp/dst naming is really
> needed?  I had wanted to keep the variable renaming and the big code
> movement in different patches to make it easier to see what's going on, but
> maybe that was misguided.

Ideally, I'd prefer dt_cg_load_var() to use 'dnp' rather than 'dst'.  So, in
merging dt_cg_assoc_op() with dt_cg_load_var() the naming assoc_op uses for
the argument should take precedence.

I am sorry I didn't catch that when dt_cg_load_var() was introduced.

> Let me know.
> 
> > > Let's keep this as-is, please.
> > 
> > I hear you, but this change isn't meant to stand on its own.  It's
> > setting up some code refactoring... in essence, the code being modified
> > here is going to disappear in later patches.  The changes in this patch
> > are supposed to be easy to review ("Oh yeah, I can see he's simply doing
> > a simple renaming") and trickier stuff will be in later patches.
> > 
> > Ultimately, the point is to combine tvar and assoc access in a way that
> > common code can be extracted to simplify the code and make it easier to
> > make changes (e.g., to support NULL strings).
> > 
> > So I get your point on this patch in isolation, but I do not understand
> > what you're suggesting for the overall roadmap.  I would think combining
> > tvar and assoc more is unequivocally a good thing.
> > 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel