dialects: (riscv) add verifiers for riscv ops using immediate #1027

adutilleul · 2023-05-30T14:03:49Z

In the RISC-V instruction format, the range of immediate values is constrained, such as a 12-bit immediate for most I-Type instructions. However, the current implementation of the dialect lacks the necessary validation for these immediate values. This pull request aims to address this issue by introducing verifiers for the operations to check that the values we pass are actually in those ranges.

The main goals of this pull request are as follows:

We ensure that the immediate values used in RISC-V instructions fall within the valid range. This verification is quite important if we want to do some code generation optimizations, such as instruction selection (e.g. merging load constant and add into addi).

An question arising from this pull request pertains to the handling of immediates in different RISC-V targets, specifically RV32I and RV64I. The current implementation assumes a RV32I target for simplicity (but in the RV64I target some same instructions use larger immediates). So any feedback on that is appreciated.

codecov · 2023-05-30T14:10:01Z

Codecov Report

Patch coverage: 96.34% and project coverage change: +0.10 🎉

Comparison is base (93bf119) 86.77% compared to head (d95574e) 86.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1027      +/-   ##
==========================================
+ Coverage   86.77%   86.88%   +0.10%     
==========================================
  Files         130      130              
  Lines       20372    20454      +82     
  Branches     3078     3095      +17     
==========================================
+ Hits        17678    17771      +93     
+ Misses       2190     2174      -16     
- Partials      504      509       +5

Impacted Files	Coverage Δ
xdsl/dialects/riscv.py	`86.55% <89.65%> (+1.81%)`	⬆️
tests/dialects/test_riscv.py	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

compor

In general, I like this since this hasn't been a consideration so far.
However, I feel that the immediate width should be a parametrized attribute (but I might be wrong) that will eventually interact with something in #980 (if this gets merged).

But for simplicity, we could just assume RV32I for now.
Curious about what other ppl think about this.

superlopuh

I would say that the verification should probably live on the attribute, not the operations. That would also allow the lowering to provide a range for the int values that they feel most appropriate for the target, while allowing the default value ranges to be for 32bit for convenience.

webmiche · 2023-05-31T09:28:22Z

From quickly scanning the code, I agree with Sasha here. This is something that should be in core xDSL rather than on the ops.

I remember having a discussion about adding this at some point, I wonder why we didn't 🤔
There certainly are some shenanigans with signedness that we would need to address, but let's maybe consider this in a different PR. Maybe this ended up being to complicated, so we decided to not do it?

adutilleul · 2023-05-31T09:39:55Z

I'll see how to implement this properly using attributes but some behaviors are quite specific to some of the RISC-V ops so adding a verifier for instance to IntegerAttr is probably not the best way of doing for all of them.

superlopuh · 2023-05-31T09:42:14Z

Yeah I'd recommend a new integer attr for riscv.

math-fehr · 2023-05-31T09:48:51Z

I would actually think the opposite, we should integrate this in IntegerAttr.
IntegerAttr should always be in range, and the reason we didn't do that is because we just didn't prioritize it cause of lack of use case.
What do you think?

superlopuh · 2023-05-31T10:00:07Z

Ah that's even better, let's improve IntegerAttr

webmiche · 2023-05-31T10:18:25Z

But what does it mean for an IntegerAttr to be in range in the context of Signless integers? [-2^bitwidth, 2^bitwidth]? Theoretically, the range has to include any kind of signedness, but unsigned and 2-complement are probably the most important ones. What about other, weird signedness concepts? 🤔

superlopuh · 2023-05-31T10:26:55Z

The verifier should be conservative, I think your range makes sense, it's not supposed to catch everything, but a one bit integer cannot store the number 100 whatever the signedness

superlopuh

I'm liking this new version a lot, and it's nice that it's already caught some bugs. Just a few things to clean up remaining

tests/dialects/test_riscv.py

xdsl/dialects/builtin.py

xdsl/dialects/riscv.py

webmiche

Maybe split this PR into adding the builtin stuff and then another one for the riscv stuff? That would make reviewing much easier for me and give a cleaner git history afterward.

From a quick look, I think the core thigns in here are fine 👍

nazavode · 2023-05-31T14:31:35Z

Thanks @adutilleul for looking into this, super useful.

But what does it mean for an IntegerAttr to be in range in the context of Signless integers? [-2^bitwidth, 2^bitwidth]? Theoretically, the range has to include any kind of signedness, but unsigned and 2-complement are probably the most important ones. What about other, weird signedness concepts? 🤔

Just my two cents: the concept of an integer range could become not enough constrained very quickly in backend jargon, i.e.: some extension instructions require that the immediate is within a range and also a power of two.

Follow-up PR of #1027 to split the changes properly.

…ect-verify

superlopuh

oh actually my comment still stands

… until fix

superlopuh · 2023-06-02T14:36:50Z

xdsl/dialects/riscv.py

-            immediate = SImm12Attr(immediate)
+            immediate = SImm12Attr(
+                immediate
+            )  # pyright: ignore[reportGeneralTypeIssues]


@adutilleul can you please copy/paste the error you get when you remove the pyright ignore?
@math-fehr do you know why we might need a type ignore here?

No overloads for "__new__" match the provided arguments Argument types: (int)Pylance[reportGeneralTypeIssues](https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportGeneralTypeIssues)
I tried to replicate the sort of "hack" that was done on IntegerAttr to fix the pyright issue but not luck so far.

@superlopuh, any idea how to resolve this?

I added another overload to IntAttr to accept no args to new. Would be good to have a tick from @math-fehr before merging

superlopuh · 2023-06-05T09:56:39Z

xdsl/dialects/riscv.py

@@ -373,7 +406,7 @@ def __init__(
        comment: str | StringAttr | None = None,
    ):
        if isinstance(immediate, int):
-            immediate = IntegerAttr.from_int_and_width(immediate, 32)
+            immediate = IntegerAttr(immediate, IntegerType(20, Signedness.SIGNED))


I'm wondering whether we might want to convert all parameters to this bitwidth. So if we get another integer attr, we'd still catch whether the bitwidth is correct. This should possibly be done at the rewrite level, though.

nazavode · 2023-06-05T13:13:16Z

I feel that the immediate width should be a parametrized attribute that will eventually interact with something in #980 [...]
But for simplicity, we could just assume RV32I for now. Curious about what other ppl think about this.

100% agree, some parametrization driven by the target will be needed at some point (e.g.: ranges for I-Type and S-Type) but for now looks great for our implicit RV32. Thanks @adutilleul

…roject#1027) In the RISC-V instruction format, the range of immediate values is constrained, such as a 12-bit immediate for most I-Type instructions. However, the current implementation of the dialect lacks the necessary validation for these immediate values. This pull request aims to address this issue by introducing verifiers for the operations to check that the values we pass are actually in those ranges. The main goals of this pull request are as follows: - We ensure that the immediate values used in RISC-V instructions fall within the valid range. This verification is quite important if we want to do some code generation optimizations, such as instruction selection (e.g. merging load constant and add into addi). An question arising from this pull request pertains to the handling of immediates in different RISC-V targets, specifically RV32I and RV64I. The current implementation assumes a RV32I target for simplicity (but in the RV64I target some same instructions use larger immediates). So any feedback on that is appreciated. --------- Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>

dialects: riscv: add some verifiers on riscv ops using immediate

be4a524

adutilleul requested review from AntonLydike, superlopuh, compor, nazavode, math-fehr, webmiche and georgebisbas as code owners May 30, 2023 14:03

adutilleul self-assigned this May 30, 2023

adutilleul added the dialects Changes on the dialects label May 30, 2023

dialects: riscv: fix formatting on the verifiers change

c32352c

compor reviewed May 30, 2023

View reviewed changes

superlopuh requested changes May 31, 2023

View reviewed changes

dialects: riscv: use attribute for verifying integer bounds in ops

d3cbc70

superlopuh requested changes May 31, 2023

View reviewed changes

tests/dialects/test_riscv.py Outdated Show resolved Hide resolved

xdsl/dialects/builtin.py Outdated Show resolved Hide resolved

xdsl/dialects/riscv.py Outdated Show resolved Hide resolved

webmiche reviewed May 31, 2023

View reviewed changes

adutilleul mentioned this pull request May 31, 2023

dialects: (builtin) add range verifier for IntegerAttr #1042

Merged

adutilleul added a commit that referenced this pull request Jun 2, 2023

dialects: (builtin) add range verifier for IntegerAttr (#1042)

90747e5

Follow-up PR of #1027 to split the changes properly.

Merge remote-tracking branch 'origin/main' into adutilleul/riscv/dial…

5501d7b

…ect-verify

superlopuh approved these changes Jun 2, 2023

View reviewed changes

superlopuh requested changes Jun 2, 2023

View reviewed changes

adutilleul added 2 commits June 2, 2023 13:58

dialects: (riscv) remove superfluous calls to verify in test

1d204c8

dialects: (riscv) replace pyright type ignore by more specific ignore…

c69b058

… until fix

superlopuh reviewed Jun 2, 2023

View reviewed changes

superlopuh added 2 commits June 4, 2023 22:24

override new

1115958

add another overload to IntegerAttr

ecac637

superlopuh approved these changes Jun 4, 2023

View reviewed changes

superlopuh added 3 commits June 5, 2023 09:58

Merge branch 'main' into adutilleul/riscv/dialect-verify

688f8d6

Merge branch 'main' into adutilleul/riscv/dialect-verify

c366a24

remove bad new

d95574e

superlopuh reviewed Jun 5, 2023

View reviewed changes

nazavode approved these changes Jun 5, 2023

View reviewed changes

adutilleul merged commit 0e73c8f into main Jun 5, 2023
12 checks passed

adutilleul deleted the adutilleul/riscv/dialect-verify branch June 5, 2023 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (riscv) add verifiers for riscv ops using immediate #1027

dialects: (riscv) add verifiers for riscv ops using immediate #1027

adutilleul commented May 30, 2023

codecov bot commented May 30, 2023 •

edited

compor left a comment

superlopuh left a comment

webmiche commented May 31, 2023

adutilleul commented May 31, 2023

superlopuh commented May 31, 2023

math-fehr commented May 31, 2023

superlopuh commented May 31, 2023

webmiche commented May 31, 2023

superlopuh commented May 31, 2023

superlopuh left a comment

webmiche left a comment

nazavode commented May 31, 2023 •

edited

superlopuh left a comment

superlopuh Jun 2, 2023

adutilleul Jun 2, 2023

tobiasgrosser Jun 4, 2023

superlopuh Jun 4, 2023

superlopuh Jun 5, 2023

nazavode commented Jun 5, 2023

dialects: (riscv) add verifiers for riscv ops using immediate #1027

dialects: (riscv) add verifiers for riscv ops using immediate #1027

Conversation

adutilleul commented May 30, 2023

codecov bot commented May 30, 2023 • edited

Codecov Report

compor left a comment

Choose a reason for hiding this comment

superlopuh left a comment

Choose a reason for hiding this comment

webmiche commented May 31, 2023

adutilleul commented May 31, 2023

superlopuh commented May 31, 2023

math-fehr commented May 31, 2023

superlopuh commented May 31, 2023

webmiche commented May 31, 2023

superlopuh commented May 31, 2023

superlopuh left a comment

Choose a reason for hiding this comment

webmiche left a comment

Choose a reason for hiding this comment

nazavode commented May 31, 2023 • edited

superlopuh left a comment

Choose a reason for hiding this comment

superlopuh Jun 2, 2023

Choose a reason for hiding this comment

adutilleul Jun 2, 2023

Choose a reason for hiding this comment

tobiasgrosser Jun 4, 2023

Choose a reason for hiding this comment

superlopuh Jun 4, 2023

Choose a reason for hiding this comment

superlopuh Jun 5, 2023

Choose a reason for hiding this comment

nazavode commented Jun 5, 2023

codecov bot commented May 30, 2023 •

edited

nazavode commented May 31, 2023 •

edited