Make trace_fmt! include commas (,) in arrays

Fixes: #1428
PiperOrigin-RevId: 636691989

Author: allight

xls/dslx/interpreter_test.py

@@ -350,6 +350,24 @@ def test_trace_fmt_array_of_enum(self):
     )
     self.assertIn('a: [MyEnum::ONE, MyEnum::TWO]', stderr)
 
+  def test_trace_fmt_array_of_ints(self):
+    """Tests we can trace-format an array of u8 values."""
+    program = """
+    fn main() {
+      let a = u8[2]:[u8:1, u8:2];
+      trace_fmt!("a: {:#x}", a);
+    }
+
+    #[test]
+    fn hello_test() {
+      main()
+    }
+    """
+    stderr = self._parse_and_test(
+        program, want_error=False, alsologtostderr=True
+    )
+    self.assertIn('a: [0x1, 0x2]', stderr)
+
   def test_trace_fmt_tuple_of_enum(self):
     """Tests that we can trace format a tuple that includes enum values."""
     program = """

xls/dslx/ir_convert/convert_format_macro.cc

@@ -92,6 +92,9 @@ absl::Status FlattenArray(const ArrayFormatDescriptor& sfd, const BValue& v,
   for (int64_t i = 0; i < sfd.size(); ++i) {
     BValue index = ctx.fn_builder.Literal(UBits(i, /*bit_count=*/32));
     BValue elem = ctx.fn_builder.ArrayIndex(v, {index});
+    if (i != 0) {
+      ctx.fmt_steps.push_back(", ");
+    }
     XLS_RETURN_IF_ERROR(Flatten(sfd.element_format(), elem, ctx));
   }
   ctx.fmt_steps.push_back("]");

xls/dslx/ir_convert/ir_converter_test.cc

@@ -1962,14 +1962,14 @@ TEST(IrConverterTest, EmptyArray) {
 
 TEST(IrConverterTest, TraceFmt) {
   constexpr std::string_view kProgram = R"(
-    fn trace_and_add(x: u32) -> u32 {
-      trace_fmt!("x = {}", x);
-      x + u32:1
+    fn trace_and_add(x: u32, y: u32[u32:2]) -> u32 {
+      trace_fmt!("x = {}, y = {}", x, y);
+      x + y[u8:1] + y[u8:0]
     }
 
     fn assert_trace_and_add(x: u32) -> u32 {
       if x == u32:5 { fail!("x_is_now_5", u32:0) } else { u32:0 };
-      trace_and_add(x)
+      trace_and_add(x, [u32:4, u32:6])
     }
 
     proc main {

xls/dslx/ir_convert/testdata/ir_converter_test_TraceFmt.ir

@@ -4,39 +4,50 @@ file_number 0 "test_module.x"
 
 chan test_module__input_c(bits[32], id=0, kind=streaming, ops=send_only, flow_control=ready_valid, strictness=proven_mutually_exclusive, metadata="""""")
 
-fn __itok__test_module__trace_and_add(__token: token, __activated: bits[1], x: bits[32]) -> (token, bits[32]) {
-  trace.4: token = trace(__token, __activated, format="x = {}", data_operands=[x], id=4)
-  literal.5: bits[32] = literal(value=1, id=5)
-  after_all.7: token = after_all(trace.4, id=7)
-  add.6: bits[32] = add(x, literal.5, id=6)
-  ret tuple.8: (token, bits[32]) = tuple(after_all.7, add.6, id=8)
+fn __itok__test_module__trace_and_add(__token: token, __activated: bits[1], x: bits[32], y: bits[32][2]) -> (token, bits[32]) {
+  literal.5: bits[32] = literal(value=0, id=5)
+  literal.7: bits[32] = literal(value=1, id=7)
+  literal.10: bits[8] = literal(value=1, id=10)
+  array_index.6: bits[32] = array_index(y, indices=[literal.5], id=6)
+  array_index.8: bits[32] = array_index(y, indices=[literal.7], id=8)
+  array_index.11: bits[32] = array_index(y, indices=[literal.10], id=11)
+  literal.13: bits[8] = literal(value=0, id=13)
+  trace.9: token = trace(__token, __activated, format="x = {}, y = [{}, {}]", data_operands=[x, array_index.6, array_index.8], id=9)
+  add.12: bits[32] = add(x, array_index.11, id=12)
+  array_index.14: bits[32] = array_index(y, indices=[literal.13], id=14)
+  after_all.16: token = after_all(trace.9, id=16)
+  add.15: bits[32] = add(add.12, array_index.14, id=15)
+  ret tuple.17: (token, bits[32]) = tuple(after_all.16, add.15, id=17)
 }
 
 fn __itok__test_module__assert_trace_and_add(__token: token, __activated: bits[1], x: bits[32]) -> (token, bits[32]) {
-  literal.12: bits[32] = literal(value=5, id=12)
-  eq.13: bits[1] = eq(x, literal.12, id=13)
-  and.16: bits[1] = and(__activated, eq.13, id=16)
-  not.17: bits[1] = not(and.16, id=17)
-  invoke.22: (token, bits[32]) = invoke(__token, __activated, x, to_apply=__itok__test_module__trace_and_add, id=22)
-  literal.15: bits[32] = literal(value=0, id=15)
-  assert.18: token = assert(__token, not.17, message="Assertion failure via fail! @ test_module.x:8:28-8:49", label="x_is_now_5", id=18)
-  tuple_index.23: token = tuple_index(invoke.22, index=0, id=23)
-  literal.20: bits[32] = literal(value=0, id=20)
-  identity.19: bits[32] = identity(literal.15, id=19)
-  after_all.25: token = after_all(assert.18, tuple_index.23, id=25)
-  tuple_index.24: bits[32] = tuple_index(invoke.22, index=1, id=24)
-  literal.14: bits[8][10] = literal(value=[120, 95, 105, 115, 95, 110, 111, 119, 95, 53], id=14)
-  sel.21: bits[32] = sel(eq.13, cases=[literal.20, identity.19], id=21)
-  ret tuple.26: (token, bits[32]) = tuple(after_all.25, tuple_index.24, id=26)
+  literal.21: bits[32] = literal(value=5, id=21)
+  eq.22: bits[1] = eq(x, literal.21, id=22)
+  literal.31: bits[32] = literal(value=4, id=31)
+  literal.32: bits[32] = literal(value=6, id=32)
+  and.25: bits[1] = and(__activated, eq.22, id=25)
+  array.33: bits[32][2] = array(literal.31, literal.32, id=33)
+  not.26: bits[1] = not(and.25, id=26)
+  invoke.34: (token, bits[32]) = invoke(__token, __activated, x, array.33, to_apply=__itok__test_module__trace_and_add, id=34)
+  literal.24: bits[32] = literal(value=0, id=24)
+  assert.27: token = assert(__token, not.26, message="Assertion failure via fail! @ test_module.x:8:28-8:49", label="x_is_now_5", id=27)
+  tuple_index.35: token = tuple_index(invoke.34, index=0, id=35)
+  literal.29: bits[32] = literal(value=0, id=29)
+  identity.28: bits[32] = identity(literal.24, id=28)
+  after_all.37: token = after_all(assert.27, tuple_index.35, id=37)
+  tuple_index.36: bits[32] = tuple_index(invoke.34, index=1, id=36)
+  literal.23: bits[8][10] = literal(value=[120, 95, 105, 115, 95, 110, 111, 119, 95, 53], id=23)
+  sel.30: bits[32] = sel(eq.22, cases=[literal.29, identity.28], id=30)
+  ret tuple.38: (token, bits[32]) = tuple(after_all.37, tuple_index.36, id=38)
 }
 
 top proc __test_module__main_0_next(__state: bits[32], init={0}) {
-  __token: token = literal(value=token, id=27)
-  literal.29: bits[1] = literal(value=1, id=29)
-  after_all.30: token = after_all(id=30)
-  invoke.32: (token, bits[32]) = invoke(__token, literal.29, __state, to_apply=__itok__test_module__assert_trace_and_add, id=32)
-  tok: token = send(after_all.30, __state, channel=test_module__input_c, id=31)
-  tuple_index.33: token = tuple_index(invoke.32, index=0, id=33)
-  tuple_index.34: bits[32] = tuple_index(invoke.32, index=1, id=34)
-  next (tuple_index.34)
+  __token: token = literal(value=token, id=39)
+  literal.41: bits[1] = literal(value=1, id=41)
+  after_all.42: token = after_all(id=42)
+  invoke.44: (token, bits[32]) = invoke(__token, literal.41, __state, to_apply=__itok__test_module__assert_trace_and_add, id=44)
+  tok: token = send(after_all.42, __state, channel=test_module__input_c, id=43)
+  tuple_index.45: token = tuple_index(invoke.44, index=0, id=45)
+  tuple_index.46: bits[32] = tuple_index(invoke.44, index=1, id=46)
+  next (tuple_index.46)
 }

xls/dslx/tests/trace_fmt_array/BUILD

@@ -0,0 +1,61 @@
+# Copyright 2024 The XLS Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+load(
+    "//xls/build_rules:xls_build_defs.bzl",
+    "cc_xls_ir_jit_wrapper",
+    "xls_dslx_ir",
+    "xls_dslx_library",
+)
+
+package(
+    default_applicable_licenses = ["//:license"],
+    default_visibility = ["//xls:xls_internal"],
+    licenses = ["notice"],  # Apache 2.0
+)
+
+xls_dslx_library(
+    name = "test_target_library",
+    srcs = ["test_target.x"],
+)
+
+xls_dslx_ir(
+    name = "test_target",
+    dslx_top = "traceit",
+    library = ":test_target_library",
+)
+
+cc_xls_ir_jit_wrapper(
+    name = "test_target_wrapper",
+    src = ":test_target.ir",
+    jit_wrapper_args = {
+        "class_name": "TraceIt",
+        "namespace": "xls::wrapped",
+    },
+)
+
+cc_test(
+    name = "trace_fmt_array_test",
+    srcs = ["trace_fmt_array_test.cc"],
+    deps = [
+        ":test_target_wrapper",
+        "//xls/common:xls_gunit_main",
+        "//xls/common/status:matchers",
+        "//xls/ir:bits",
+        "//xls/ir:events",
+        "//xls/ir:value",
+        "//xls/ir:value_builder",
+        "@com_google_googletest//:gtest",
+    ],
+)

xls/dslx/tests/trace_fmt_array/test_target.x

@@ -0,0 +1,18 @@
+// Copyright 2024 The XLS Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+fn traceit(v: u8[8]) -> u8 {
+    trace_fmt!("The array is {}", v);
+    v[0]
+}

xls/dslx/tests/trace_fmt_array/trace_fmt_array_test.cc

@@ -0,0 +1,48 @@
+// Copyright 2024 The XLS Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "xls/common/status/matchers.h"
+#include "xls/dslx/tests/trace_fmt_array/test_target_wrapper.h"
+#include "xls/ir/bits.h"
+#include "xls/ir/events.h"
+#include "xls/ir/value.h"
+#include "xls/ir/value_builder.h"
+
+namespace xls {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+MATCHER_P(TraceMessage, m, "") {
+  return testing::ExplainMatchResult(m, arg.message, result_listener);
+}
+TEST(TraceFmtArray, TraceHasCommas) {
+  XLS_ASSERT_OK_AND_ASSIGN(auto trace, wrapped::TraceIt::Create());
+  XLS_ASSERT_OK_AND_ASSIGN(
+      auto input,
+      ValueBuilder::UBitsArray({1, 2, 3, 4, 5, 6, 7, 8}, 8).Build());
+  XLS_ASSERT_OK_AND_ASSIGN(InterpreterResult<Value> res,
+                           trace->jit()->Run({input}));
+
+  EXPECT_EQ(res.value.bits(), UBits(1, 8));
+  EXPECT_THAT(res.events.assert_msgs, IsEmpty());
+  EXPECT_THAT(
+      res.events.trace_msgs,
+      ElementsAre(TraceMessage("The array is [1, 2, 3, 4, 5, 6, 7, 8]")));
+}
+
+}  // namespace
+}  // namespace xls