Replace autogen DIF code by bazel rules #25775

pamaury · 2025-01-06T10:26:32Z

This PR changes the way we manage the autogenerated part of the DIFs: instead of checking-in the autogenerated files, which is not super compatible with multitop, move those to bazel rules. This requires to split make_new_dif into two: the actual autogen part, and the part generating actually new DIFs.

Razer6

Thanks @pamaury. This looks good to me.

util/autogen_dif.py

a-will

LGTM

I like that some of our path assumptions are removed here 😄

util/make_new_dif/ip.py

jwnrt

LGTM, thanks

util/autogen_testutils.py

util/make_new_dif.py

util/autogen_testutils.py

util/autogen_dif.py

matutem · 2025-01-13T23:43:23Z

rules/autogen.bzl

+            """,
+        ),
+        "_autogen_dif": attr.label(
+            default = "//util:autogen_dif",


Nt: autogen_dif is a misnomer since there is nothing "auto" about invoking a tool to generate something. "gen_dif" is better, but I know "autogen" is unfortunately pervasive in our project.

This tool is invoked by bazel which I guess is why it was called autogen to start with. But generally I agree that gen would be better.

matutem · 2025-01-13T23:50:48Z

Amaury, I left some comments. You can ignore some of the Nits :)

It is not clear this deals with the path change for ipgens, and for these did you fix the path in the href near the top in files like dif_clkmgr_autogen.h? It should refer to a specific top.

matutem · 2025-01-15T04:24:34Z

util/autogen_dif.py

+
+# This file is $REPO_TOP/util/make_new_dif.py, so it takes two parent()
+# calls to get back to the top.
+REPO_TOP = Path(__file__).resolve().parent.parent


Suggested change

REPO_TOP = Path(__file__).resolve().parent.parent

REPO_TOP = Path(__file__).resolve().parents[1]

I think repo_top could be provided by a library like util/base/lib.py, since all sorts of tools get it in all sorts of ways, and we could make a single more reliable way to get it. It could, for example, at least check that REPO_TOP / "util" is_dir().

A potentially more stable way to get REPO_TOP is from git, as in https://github.com/lowRISC/opentitan/blob/master/util/dvsim/dvsim.py#L154, but that fires a new subprocess.

I don't think relying on git is good idea because if someone downloads the code as a tarball checkout, this will make the tool fail for no good reason. I generally agree that this way of doing things is ugly but I think the issue that our python codebase is fundamentally broken in how it is organized.
Anyway, I have removed this particular REPO_TOP as it turns out I could do without.

matutem · 2025-01-15T04:44:03Z

util/topgen/lib.py

@@ -368,6 +368,23 @@ def search_ips(ip_path):  # return list of config files
    return ips


+def get_ip_hjson_path(ip_name_snake: str, topcfg: Dict[str, object], repotop: Path) -> Path:


This will traverse through all modules twice and generate lists, once for ipgens, once for reggens, and then the "in" operator on lists is also O(N).

It is probably much better to get the module with lib.find_module(ip_name_snake) and then decide based on lib.is_ipgen and lib.is_top_reggen.

matutem

The REPO_TOP comment is up to you, but I think creating a library function to retrieve repo_top will be good.

I would encourage you to take the comment about get_ip_hjson_path more seriously. Topgen uses code like I suggest and it is way more efficient and clean.

Otherwise LGTM

Currently, the code assumes earlgrey by hardcoding some paths in the library. This commit changes the code to remove assumption and directly take the hjson path in. The leaves the path guessing dance to the frontend. Signed-off-by: Amaury Pouly <[email protected]>

pamaury · 2025-01-15T11:00:53Z

The REPO_TOP comment is up to you, but I think creating a library function to retrieve repo_top will be good.

I would encourage you to take the comment about get_ip_hjson_path more seriously. Topgen uses code like I suggest and it is way more efficient and clean.

Otherwise LGTM

I have address your comment about get_ip_hjson_path. I also fixed the one instance of REPO_TOP that you pointed to but I honestly don't know what the general solution for REPO_TOP should be so I prefer not to touch it for now. Maybe a solution would be:

start from __file__
walk up until you find MODULE.bazel

What do you think?

I also fixed an issue with autogen_testutils.py. I think this tool is not used by anything? It keeps breaking (it was broken before this PR) without noticing.

Instead of hardcoding REPO_TOP in everything, this library provides a generic way of finding the path. Signed-off-by: Amaury Pouly <[email protected]>

This tool is basically extracted from make_new_dif and focuses on generating the dif_<ip>.{c,h} and _unitest.cc files for a given IP. This tool will be used in conjunction with bazel to generate those files. Signed-off-by: Amaury Pouly <[email protected]>

Those files are currently generated by make_new_dif and checked-in the repository. With this rule, we will switch to generating them dynamically. Signed-off-by: Amaury Pouly <[email protected]>

Signed-off-by: Amaury Pouly <[email protected]>

In the next commit, we will remove all the autogenerated files in sw/device/lib/dif/autogen. This will break how autogen_testutils detects IPs with diff. The logic is change to the following: go through the list of IPs for the top of interest, check in sw/device/lib/dif if there is a file called dif_<ip>.h. This produces exactly the same result. Signed-off-by: Amaury Pouly <[email protected]>

There are not useful anymore. Signed-off-by: Amaury Pouly <[email protected]>

Signed-off-by: Amaury Pouly <[email protected]>

This functionality has been move to autogen_dif so simplify make_new_dif to actually... make NEW difs! The command line is also changed: instead of requiring a top-level cfg file (which makes little sense and pulls it topgen.lib), require to point to the IP's hjson directly. Signed-off-by: Amaury Pouly <[email protected]>

pamaury · 2025-01-15T12:24:36Z

On second thought, I have introduce a new library, repo_top.py to find the top. It currently it only used in autogen_dif.py

matutem · 2025-01-16T17:00:14Z

Thanks for adding the detection of repo_top. We can improve on this now, like recursing down till MODULE.bazel, BLOCKFILE, and/or util are found as you mention.

pamaury · 2025-01-16T17:43:57Z

The CI failures are unrelated to this PR. The check for unauthorized changes is failing because the PR changes too many files but this PR does not actually change any unauthorized files.

pamaury force-pushed the multitop_dif branch 2 times, most recently from 5770b36 to abeb7fa Compare January 6, 2025 14:23

pamaury requested review from cfrantz, a-will, andreaskurth, matutem and jwnrt January 6, 2025 14:35

pamaury marked this pull request as ready for review January 6, 2025 15:48

pamaury requested review from rswarbrick, msfschaffner, vogelpi and a team as code owners January 6, 2025 15:48

pamaury requested review from moidx and Razer6 and removed request for a team January 6, 2025 15:48

Razer6 approved these changes Jan 8, 2025

View reviewed changes

util/autogen_dif.py Outdated Show resolved Hide resolved

pamaury force-pushed the multitop_dif branch 2 times, most recently from a373123 to f0eaf86 Compare January 10, 2025 14:04

a-will approved these changes Jan 10, 2025

View reviewed changes

util/make_new_dif/ip.py Outdated Show resolved Hide resolved

jwnrt approved these changes Jan 13, 2025

View reviewed changes

pamaury force-pushed the multitop_dif branch from f0eaf86 to 5ba317a Compare January 13, 2025 16:14