[PATCH 3/3] cg: optimize ternary expressions for strings

[PATCH 3/3] cg: optimize ternary expressions for strings

[Kris Van Hees]

If either size of a ternary expression has a tstring value, it can be
re-used to store the value of the ternary expression, reducing the
need for tstring allocation, especially in nested ternaries.

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 libdtrace/dt_cg.c                             | 56 +++++++++++++++----
 libdtrace/dt_impl.h                           |  2 +-
 .../codegen/tst.tstring_ternary_mix.d         | 30 ++++++++++
 .../codegen/tst.tstring_ternary_mix.r         |  1 +
 .../codegen/tst.tstring_ternary_nested.d      | 26 +++++++++
 .../codegen/tst.tstring_ternary_nested.r      |  1 +
 6 files changed, 105 insertions(+), 11 deletions(-)
 create mode 100644 test/unittest/codegen/tst.tstring_ternary_mix.d
 create mode 100644 test/unittest/codegen/tst.tstring_ternary_mix.r
 create mode 100644 test/unittest/codegen/tst.tstring_ternary_nested.d
 create mode 100644 test/unittest/codegen/tst.tstring_ternary_nested.r

diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
index 6c7ad076..11c94a5e 100644
--- a/libdtrace/dt_cg.c
+++ b/libdtrace/dt_cg.c
@@ -4630,29 +4630,65 @@ dt_cg_ternary_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
 	 * dn_right).
 	 */
 	if (dt_node_is_string(dnp)) {
-		uint_t lbl_null = dt_irlist_label(dlp);
+		uint_t	lbl_done = dt_irlist_label(dlp);
+		int	left_is_tstr, rght_is_tstr;
 
-		emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, dnp->dn_reg, 0, lbl_null));
+		emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, dnp->dn_reg, 0, lbl_done));
 
 		/*
-		 * At this point, dnp->dn_reg holds a pointer to the string we
-		 * need to copy.  But we want to copy it into a tstring which
-		 * location is to be stored in dnp->dn_reg.  So, we need to
-		 * shuffle things a bit.
+		 * At this point, dnp->dn_reg holds a pointer to the string to
+		 * be used as value of the ternary.  It needs to be made
+		 * available as a tstring.
+		 * If dnp->dn_left is a tstring, reuse it as tstring for the
+		 * ternary value.
+		 * If dnp->dn_left is not a tstring but dnp->dn_right is, use
+		 * that one as tstring for the ternary value.
+		 * Otherwise, allocate a tstring.
+		 *
+		 * Either way, we copy the value (a string) into the tstring
+		 * (unless it is already there).
 		 */
 		emit(dlp,  BPF_MOV_REG(BPF_REG_0, dnp->dn_reg));
-		dt_cg_tstring_alloc(yypcb, dnp);
+
+		left_is_tstr = dt_node_is_tstring(dnp->dn_left);
+		rght_is_tstr = dt_node_is_tstring(dnp->dn_right);
+
+		if (left_is_tstr)
+			dnp->dn_tstring = dnp->dn_left->dn_tstring;
+		else if (rght_is_tstr)
+			dnp->dn_tstring = dnp->dn_right->dn_tstring;
+		else
+			dt_cg_tstring_alloc(yypcb, dnp);
 
 		dt_cg_access_dctx(dnp->dn_reg, dlp, drp, DCTX_MEM);
 		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, dnp->dn_tstring->dn_value));
 
+		emit(dlp,  BPF_BRANCH_REG(BPF_JEQ, dnp->dn_reg, BPF_REG_0, lbl_done));
+
 		dt_cg_memcpy(dlp, drp, dnp->dn_reg, BPF_REG_0,
 			     yypcb->pcb_hdl->dt_options[DTRACEOPT_STRSIZE]);
 
-		emitl(dlp, lbl_null,
+		emitl(dlp, lbl_done,
 			   BPF_NOP());
-		dt_cg_tstring_free(yypcb, dnp->dn_left);
-		dt_cg_tstring_free(yypcb, dnp->dn_right);
+
+		/*
+		 * Free the (possible) tstrings for left and right children.
+		 * If a child is a tstring and that tstring is being reused for
+		 * the ternary value, simply set dn_tstring to NULL.  Otherwise
+		 * truly free it.
+		 */
+		if (left_is_tstr) {
+			if (dnp->dn_tstring != dnp->dn_left->dn_tstring)
+				dt_cg_tstring_free(yypcb, dnp->dn_left);
+			else
+				dnp->dn_left->dn_tstring = NULL;
+		}
+		if (rght_is_tstr) {
+			if (dnp->dn_tstring != dnp->dn_right->dn_tstring)
+				dt_cg_tstring_free(yypcb, dnp->dn_right);
+			else
+				dnp->dn_right->dn_tstring = NULL;
+		}
 	}
 }
 
diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
index 2adc1252..c6d9e782 100644
--- a/libdtrace/dt_impl.h
+++ b/libdtrace/dt_impl.h
@@ -222,7 +222,7 @@ typedef struct dt_kern_path {
  * - cleanpath() holds a prepended '/' char, a string, an appended '/' char,
  *   and a terminating NUL char, or STRSZ + 3 chars altogether
  */
-#define DT_TSTRING_SLOTS	4
+#define DT_TSTRING_SLOTS	6
 #define DT_TSTRING_SIZE(dtp)	\
 		MAX(P2ROUNDUP((dtp)->dt_options[DTRACEOPT_STRSIZE] + 3, 8), \
 		    72)
diff --git a/test/unittest/codegen/tst.tstring_ternary_mix.d b/test/unittest/codegen/tst.tstring_ternary_mix.d
new file mode 100644
index 00000000..0123c428
--- /dev/null
+++ b/test/unittest/codegen/tst.tstring_ternary_mix.d
@@ -0,0 +1,30 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
+ * Licensed under the Universal Permissive License v 1.0 as shown at
+ * http://oss.oracle.com/licenses/upl.
+ */
+
+/*
+ * Ensure tstrings are handled correctly for ternary (?:) expressions.
+ */
+
+#pragma D option quiet
+
+BEGIN {
+	a = 1;
+	b = 2;
+	trace(a != b ? "abcdef" : "ABCDEF");
+	trace(" ");
+	trace(a > b ? strjoin("abc", "def") : "ABCDEF");
+	trace(" ");
+	trace(a < b ? "abcdef" : strjoin("ABC", "DEF"));
+	trace(" ");
+	trace(a == b ? strjoin("abc", "def") : strjoin("ABC", "DEF"));
+
+	exit(0);
+}
+
+ERROR {
+	exit(1);
+}
diff --git a/test/unittest/codegen/tst.tstring_ternary_mix.r b/test/unittest/codegen/tst.tstring_ternary_mix.r
new file mode 100644
index 00000000..72cc85c3
--- /dev/null
+++ b/test/unittest/codegen/tst.tstring_ternary_mix.r
@@ -0,0 +1 @@
+abcdef ABCDEF abcdef ABCDEF
diff --git a/test/unittest/codegen/tst.tstring_ternary_nested.d b/test/unittest/codegen/tst.tstring_ternary_nested.d
new file mode 100644
index 00000000..8cd2ea60
--- /dev/null
+++ b/test/unittest/codegen/tst.tstring_ternary_nested.d
@@ -0,0 +1,26 @@
+/*
+ * Oracle Linux DTrace.
+ * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
+ * Licensed under the Universal Permissive License v 1.0 as shown at
+ * http://oss.oracle.com/licenses/upl.
+ */
+
+/*
+ * Ensure tstrings are handled correctly for nested ternary (?:) expressions.
+ */
+
+#pragma D option quiet
+
+BEGIN {
+	x = 42;
+	trace(x > 1 ? strjoin(strjoin("a", "bc"), strjoin("de", "f")) :
+	      x > 2 ? strjoin(strjoin("A", "BC"), strjoin("DE", "F")) :
+	      x > 3 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
+	      strjoin(strjoin("U", "VW"), strjoin("XY", "Z")));
+
+	exit(0);
+}
+
+ERROR {
+	exit(1);
+}
diff --git a/test/unittest/codegen/tst.tstring_ternary_nested.r b/test/unittest/codegen/tst.tstring_ternary_nested.r
new file mode 100644
index 00000000..0373d933
--- /dev/null
+++ b/test/unittest/codegen/tst.tstring_ternary_nested.r
@@ -0,0 +1 @@
+abcdef
-- 
2.45.2

Re: [DTrace-devel] [PATCH 3/3] cg: optimize ternary expressions for strings

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
subject to a few comments below.

On 7/15/25 15:50, Kris Van Hees via DTrace-devel wrote:

> If either size of a ternary expression has a tstring value, it can be

s/size/side/

> re-used to store the value of the ternary expression, reducing the
> need for tstring allocation, especially in nested ternaries.
>
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> @@ -222,7 +222,7 @@ typedef struct dt_kern_path {
>    * - cleanpath() holds a prepended '/' char, a string, an appended '/' char,
>    *   and a terminating NUL char, or STRSZ + 3 chars altogether
>    */
> -#define DT_TSTRING_SLOTS	4
> +#define DT_TSTRING_SLOTS	6

Same comment as I had for Alan's patch.  The comment block before this 
goes into excruciating detail about why the value should be 4. Whether 
its logic is right or wrong, we cannot leave those old comments with a 
new value.  Why is the value now 6?

I assume 6 is not a "sufficient for all purposes" value.  E.g., if I 
kick up the complexity on tst.tstring_ternary_nested.d, I get:

$ git diff
diff --git a/test/unittest/codegen/tst.tstring_ternary_nested.d 
b/test/unittest/codegen/tst.tstring_ternary_nested.d
@@ -16,6 +16,8 @@ BEGIN {
         trace(x > 1 ? strjoin(strjoin("a", "bc"), strjoin("de", "f")) :
               x > 2 ? strjoin(strjoin("A", "BC"), strjoin("DE", "F")) :
               x > 3 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
+             x > 4 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
+             x > 5 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
               strjoin(strjoin("U", "VW"), strjoin("XY", "Z")));

         exit(0);
$ sudo ./runtest.sh test/unittest/codegen/tst.tstring_ternary_nested.d
test/unittest/codegen/tst.tstring_ternary_nested.d: FAIL: core dumped.
1 cases (0 PASS, 1 FAIL, 0 XPASS, 0 XFAIL, 0 SKIP)
$ cat test/log/current/runtest.log
[...]
dtrace: libdtrace/dt_cg.c:1472: dt_cg_tstring_xalloc: Assertion `i < 
DT_TSTRING_SLOTS' failed.

> diff --git a/test/unittest/codegen/tst.tstring_ternary_nested.d b/test/unittest/codegen/tst.tstring_ternary_nested.d
> @@ -0,0 +1,26 @@
> +BEGIN {
> +	x = 42;
> +	trace(x > 1 ? strjoin(strjoin("a", "bc"), strjoin("de", "f")) :
> +	      x > 2 ? strjoin(strjoin("A", "BC"), strjoin("DE", "F")) :
> +	      x > 3 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
> +	      strjoin(strjoin("U", "VW"), strjoin("XY", "Z")));
> +
> +	exit(0);
> +}

I guess that's fine for cg, but the skeptic in me notices that the three 
tests all evaluate to true.  I would think one could run through all 
2x2x2=8 cases in a single test for a little bit more rigor.

Re: [DTrace-devel] [PATCH 3/3] cg: optimize ternary expressions for strings

[Kris Van Hees]

On Wed, Jul 16, 2025 at 01:00:43AM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> subject to a few comments below.
> 
> On 7/15/25 15:50, Kris Van Hees via DTrace-devel wrote:
> 
> > If either size of a ternary expression has a tstring value, it can be
> 
> s/size/side/

Thanks.

> > re-used to store the value of the ternary expression, reducing the
> > need for tstring allocation, especially in nested ternaries.
> > 
> > diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> > @@ -222,7 +222,7 @@ typedef struct dt_kern_path {
> >    * - cleanpath() holds a prepended '/' char, a string, an appended '/' char,
> >    *   and a terminating NUL char, or STRSZ + 3 chars altogether
> >    */
> > -#define DT_TSTRING_SLOTS	4
> > +#define DT_TSTRING_SLOTS	6
> 
> Same comment as I had for Alan's patch.  The comment block before this goes
> into excruciating detail about why the value should be 4. Whether its logic
> is right or wrong, we cannot leave those old comments with a new value.  Why
> is the value now 6?

Ah yes, I should have updated the explanation.  It has become a little bit more
complicated but I can certainly explain it based on the example we have in the
testsuite.
> 
> I assume 6 is not a "sufficient for all purposes" value.  E.g., if I kick up
> the complexity on tst.tstring_ternary_nested.d, I get:

Correct - I hope to do a later patch that makes this something that adjusts
dynamically.  But that is quite a bit more work.
> 
> $ git diff
> diff --git a/test/unittest/codegen/tst.tstring_ternary_nested.d
> b/test/unittest/codegen/tst.tstring_ternary_nested.d
> @@ -16,6 +16,8 @@ BEGIN {
>         trace(x > 1 ? strjoin(strjoin("a", "bc"), strjoin("de", "f")) :
>               x > 2 ? strjoin(strjoin("A", "BC"), strjoin("DE", "F")) :
>               x > 3 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
> +             x > 4 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
> +             x > 5 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
>               strjoin(strjoin("U", "VW"), strjoin("XY", "Z")));
> 
>         exit(0);
> $ sudo ./runtest.sh test/unittest/codegen/tst.tstring_ternary_nested.d
> test/unittest/codegen/tst.tstring_ternary_nested.d: FAIL: core dumped.
> 1 cases (0 PASS, 1 FAIL, 0 XPASS, 0 XFAIL, 0 SKIP)
> $ cat test/log/current/runtest.log
> [...]
> dtrace: libdtrace/dt_cg.c:1472: dt_cg_tstring_xalloc: Assertion `i <
> DT_TSTRING_SLOTS' failed.
> 
> > diff --git a/test/unittest/codegen/tst.tstring_ternary_nested.d b/test/unittest/codegen/tst.tstring_ternary_nested.d
> > @@ -0,0 +1,26 @@
> > +BEGIN {
> > +	x = 42;
> > +	trace(x > 1 ? strjoin(strjoin("a", "bc"), strjoin("de", "f")) :
> > +	      x > 2 ? strjoin(strjoin("A", "BC"), strjoin("DE", "F")) :
> > +	      x > 3 ? strjoin(strjoin("u", "vw"), strjoin("xy", "z")) :
> > +	      strjoin(strjoin("U", "VW"), strjoin("XY", "Z")));
> > +
> > +	exit(0);
> > +}
> 
> I guess that's fine for cg, but the skeptic in me notices that the three
> tests all evaluate to true.  I would think one could run through all 2x2x2=8
> cases in a single test for a little bit more rigor.

Since this is a codegen test, we only really care about the tstring allocation
aspect.  There are already other tests that exercise the ternary itself.