transformations: (convert-snrt-to-riscv) Add lowerings for even more info ops #2428

AntonLydike · 2024-04-04T13:59:29Z

Missing ops:

snrt.global_compute_core_idx
snrt.global_compute_core_num
snrt.global_dm_core_num
snrt.cluster_compute_core_idx
snrt.cluster_dm_core_idx

These seem to be relatively unused in practice and are skipped for now.

AntonLydike · 2024-04-04T14:04:20Z

The is_dm_core and is_compute_core return values have been changed to i1 from i32 such that they can be used as conditionals without further hassle

codecov · 2024-04-04T14:06:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.65%. Comparing base (81fc104) to head (26d847d).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2428      +/-   ##
==========================================
- Coverage   89.77%   89.65%   -0.13%     
==========================================
  Files         354      357       +3     
  Lines       44705    45156     +451     
  Branches     6704     6788      +84     
==========================================
+ Hits        40136    40486     +350     
- Misses       3592     3651      +59     
- Partials      977     1019      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AntonLydike · 2024-04-04T14:06:38Z

This also adds some stuff that uses compile time constants in the snitch repo in the SnrtConstants dataclass

JosseVanDelm

Looks good, but it seems a bit arbitrary to only give the cluster number as a parameter to this pass?
Can you also add some more documentation to the pass arguments?

JosseVanDelm · 2024-04-04T14:59:54Z

xdsl/transforms/convert_snrt_to_riscv.py

+    """
+
+    SNRT_CLUSTER_NUM: int
+    SNRT_CLUSTER_CORE_NUM: int = 9  # TODO: is this correct?


That is defined here:
https://github.com/pulp-platform/snitch_cluster/blob/883a0ab3ed67f1a16ec920d5e985ae2a0eba5003/target/snitch_cluster/sw/runtime/common/snitch_cluster_cfg.h.tpl

And is based on the config used (default):
https://github.com/pulp-platform/snitch_cluster/blob/883a0ab3ed67f1a16ec920d5e985ae2a0eba5003/target/snitch_cluster/cfg/default.hjson

JosseVanDelm · 2024-04-04T15:01:11Z

xdsl/transforms/convert_snrt_to_riscv.py

+    ):
+        rewriter.replace_matched_op(
+            [
+                nfo := riscv.LiOp(self.constants.SNRT_BASE_HARTID),


Should this be info?

JosseVanDelm · 2024-04-04T15:04:22Z

xdsl/transforms/convert_snrt_to_riscv.py

 class ConvertSnrtToRISCV(ModulePass):
    name = "convert-snrt-to-riscv"

+    cluster_num: int


superlopuh · 2024-04-04T16:29:25Z

Is there any way you could split this up?

AntonLydike · 2024-04-04T16:39:30Z

Looks good, but it seems a bit arbitrary to only give the cluster number as a parameter to this pass? Can you also add some more documentation to the pass arguments?

I felt at the time that the cluster number is the only "real" variable part, but thinking about it more, snax has different counts for dm and compute cores...

JosseVanDelm · 2024-04-05T14:47:08Z

I felt at the time that the cluster number is the only "real" variable part, but thinking about it more, snax has different counts for dm and compute cores...

To me it seems more logical if you keep all of these being defined as constants for the time being instead of any particular one being targettable.
Adding a proper parser for these options is probably a bit overkill and would introduce new dependencies on hjson/mako (?)

(hopefully) smaller version of #2428 stripped of all the more complex lowerings introduced in that PR

Missing ops: - snrt.global_compute_core_idx - snrt.global_compute_core_num - snrt.global_dm_core_num - snrt.cluster_compute_core_idx - snrt.cluster_dm_core_idx These seem to be relatively unused in practice and are skipped for now

AntonLydike · 2024-05-10T15:41:26Z

After merging #2559, this PR should be reviewable now @superlopuh

AntonLydike · 2024-05-10T15:43:10Z

This PR now adds these 5 operations, each to a bit more complex set of operations:

snrt.cluster_idx
snrt.cluster_core_idx
snrt.global_core_idx
snrt.is_compute_core
snrt.is_dm_core

superlopuh · 2024-05-11T10:11:00Z

xdsl/transforms/convert_snrt_to_riscv.py

@@ -536,6 +675,8 @@ class ConvertSnrtToRISCV(SnrtConstants, ModulePass):

    name = "convert-snrt-to-riscv"

+    cluster_num: int


Suggested change

cluster_num: int

AntonLydike added the transformations Changes or adds a transformatio label Apr 4, 2024

AntonLydike requested review from superlopuh, JosseVanDelm, KGrykiel and cappadokes April 4, 2024 13:59

AntonLydike self-assigned this Apr 4, 2024

compor self-requested a review April 4, 2024 14:55

JosseVanDelm reviewed Apr 4, 2024

View reviewed changes

AntonLydike force-pushed the anton/snrt_more_lowerings branch from 8bba923 to 0e31ef2 Compare May 10, 2024 13:58

AntonLydike mentioned this pull request May 10, 2024

transforms: (convert-snrt-to-riscv) Add smaller number of ops #2559

Merged

AntonLydike added a commit that referenced this pull request May 10, 2024

transforms: (convert-snrt-to-riscv) Add smaller number of ops (#2559)

81fc104

(hopefully) smaller version of #2428 stripped of all the more complex lowerings introduced in that PR

AntonLydike force-pushed the anton/snrt_more_lowerings branch from 0e31ef2 to 930d009 Compare May 10, 2024 15:39

AntonLydike requested a review from JosseVanDelm May 10, 2024 15:43

add docstrings to snrt constants

70f1eed

AntonLydike changed the title ~~transformations: (convert-snrt-to-riscv) Add lowerings for almost all info ops~~ transformations: (convert-snrt-to-riscv) Add lowerings for even more info ops May 10, 2024

superlopuh reviewed May 11, 2024

View reviewed changes

superlopuh approved these changes May 11, 2024

View reviewed changes

remove unused attribute

26d847d

AntonLydike merged commit ad73bdb into main May 13, 2024
10 checks passed

AntonLydike deleted the anton/snrt_more_lowerings branch May 13, 2024 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transformations: (convert-snrt-to-riscv) Add lowerings for even more info ops #2428

transformations: (convert-snrt-to-riscv) Add lowerings for even more info ops #2428

AntonLydike commented Apr 4, 2024

AntonLydike commented Apr 4, 2024

codecov bot commented Apr 4, 2024 •

edited

Loading

AntonLydike commented Apr 4, 2024

JosseVanDelm left a comment

JosseVanDelm Apr 4, 2024

JosseVanDelm Apr 4, 2024

JosseVanDelm Apr 4, 2024

AntonLydike May 10, 2024

superlopuh commented Apr 4, 2024

AntonLydike commented Apr 4, 2024

JosseVanDelm commented Apr 5, 2024

AntonLydike commented May 10, 2024

AntonLydike commented May 10, 2024

superlopuh May 11, 2024

		@@ -536,6 +675,8 @@ class ConvertSnrtToRISCV(SnrtConstants, ModulePass):

		name = "convert-snrt-to-riscv"

		cluster_num: int

transformations: (convert-snrt-to-riscv) Add lowerings for even more info ops #2428

transformations: (convert-snrt-to-riscv) Add lowerings for even more info ops #2428

Conversation

AntonLydike commented Apr 4, 2024

AntonLydike commented Apr 4, 2024

codecov bot commented Apr 4, 2024 • edited Loading

Codecov Report

AntonLydike commented Apr 4, 2024

JosseVanDelm left a comment

Choose a reason for hiding this comment

JosseVanDelm Apr 4, 2024

Choose a reason for hiding this comment

JosseVanDelm Apr 4, 2024

Choose a reason for hiding this comment

JosseVanDelm Apr 4, 2024

Choose a reason for hiding this comment

AntonLydike May 10, 2024

Choose a reason for hiding this comment

superlopuh commented Apr 4, 2024

AntonLydike commented Apr 4, 2024

JosseVanDelm commented Apr 5, 2024

AntonLydike commented May 10, 2024

AntonLydike commented May 10, 2024

superlopuh May 11, 2024

Choose a reason for hiding this comment

codecov bot commented Apr 4, 2024 •

edited

Loading