dialects: (riscv) add fastmath flag to RdRsRs Float Float Int operations #3276

knickish · 2024-10-10T01:44:46Z

riscv-only part of #3272, with a new dialect test added. Thanks for the suggestion of separate tests, I didn't realize those existed and caught an error in the LiOp implementation while adding the test cases.

codecov · 2024-10-10T01:50:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.01%. Comparing base (f2ba5f7) to head (729668d).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3276   +/-   ##
=======================================
  Coverage   90.01%   90.01%           
=======================================
  Files         445      445           
  Lines       55850    55926   +76     
  Branches     5372     5358   -14     
=======================================
+ Hits        50272    50341   +69     
- Misses       4169     4177    +8     
+ Partials     1409     1408    -1

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

alexarice · 2024-10-10T09:08:09Z

xdsl/dialects/riscv.py

@@ -2491,6 +2491,7 @@ class LiOp(RISCVInstruction, ABC):

    rd = result_def(IntRegisterType)
    immediate = attr_def(base(Imm32Attr) | base(LabelAttr))
+    fastmath = opt_attr_def(FastMathFlagsAttr)


Is there a specific reason this is an attribute here where it was a property on the other PR?

I would recommend changing to a property here

alexarice · 2024-10-10T09:10:03Z

xdsl/dialects/riscv.py

@@ -2527,12 +2530,19 @@ def assembly_line_args(self) -> tuple[AssemblyInstructionArg, ...]:
    def custom_parse_attributes(cls, parser: Parser) -> dict[str, Attribute]:
        attributes = dict[str, Attribute]()
        attributes["immediate"] = parse_immediate_value(parser, i32)
+        fast = FastMathFlagsAttr("none")


Is this flag meant to be optional? It seems you can't parse it with the flag unset?

As on the other PR, it's not meant to be optional, op_prop_def just doesn't actually use the default parameter at all, so parsing fails when it's set as a non-opt prop_def

I would recommend making it non-optional, and to do the default value handling manually. What's the error when you change the declaration to prop_def?

I guess this is more a question for the equivalent line below, if we're removing the LiOp changes

# | raise VerifyException(f"property {prop_name} expected") # | xdsl.utils.exceptions.VerifyException: property fastmath expected

That's the error I get, even with setting a default via `fastmath = prop_def(FastMathFlagsAttr, default=FastMathFlagsAttr("none")) (which also doesn't typecheck)

yep, the default field here doesn't work. I think @alexarice is now looking into this, but for the time being we can just set it directly in the init and in the custom parser. So all that would be necessary in this PR is to change opt_prop_def to prop_def

I've just pushed a commit that does exactly that, but it's failing the filecheck test with the same error as above. Is there something else I need to do ? I am setting the fastmath property in both the parse and init methods

alexarice · 2024-10-10T09:11:15Z

xdsl/dialects/riscv.py

+    rd = result_def(IntRegisterType)
+    rs1 = operand_def(FloatRegisterType)
+    rs2 = operand_def(FloatRegisterType)
+    fastmath = opt_attr_def(FastMathFlagsAttr)


Similar comments here about attr/prop and this being optional

xdsl/dialects/riscv.py

superlopuh · 2024-10-10T09:16:01Z

xdsl/dialects/riscv.py

@@ -2491,6 +2491,7 @@ class LiOp(RISCVInstruction, ABC):

    rd = result_def(IntRegisterType)
    immediate = attr_def(base(Imm32Attr) | base(LabelAttr))
+    fastmath = opt_attr_def(FastMathFlagsAttr)


What is the semantics change of li being optionally fastmath?

Probably worth documenting

I spent a while trying to find any information on this, and I'm not seeing that there is any actual effect on the li instr at all when compiled with fastmath. Chapter 15. Load Immediate of the riscv-asm-manual doesn't mention fastmath at all, and godbolt doesn't seem to show any difference when compiled with/without -ffast-math.

Is there anywhere else that I should look for information on this? If not, should I go ahead and remove the fastmath attribute from LiOp?

I also don't know how it would affect anything. I don't think arith.constant can have fastmath, can it? It seems safe to remove from this PR, and add it later if we find a way to leverage it.

I've removed it now, and rebased everything into one commit since it was getting messy

knickish · 2024-10-10T16:31:57Z

Ok thanks, will do this stuff this evening hopefully

Need to:

Make fastmath a property rather than attribute on both op types
~~Document semantic changes to LiOp now that it has the optional fastmath property~~

…, with filecheck test

xdsl/dialects/riscv.py

superlopuh

Thank you!

`backend`(lowering)-only part of #3272. Depends on #3275 and #3276. This should close #2725 once merged Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>

…ons (xdslproject#3276) `riscv`-only part of xdslproject#3272, with a new dialect test added. Thanks for the suggestion of separate tests, I didn't realize those existed and caught an error in the LiOp implementation while adding the test cases.

`backend`(lowering)-only part of xdslproject#3272. Depends on xdslproject#3275 and xdslproject#3276. This should close xdslproject#2725 once merged Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>

knickish force-pushed the riscv_cmpf_fastmath branch from 3e230b3 to 1af6acd Compare October 10, 2024 01:45

knickish force-pushed the riscv_cmpf_fastmath branch from 1af6acd to 4c18a73 Compare October 10, 2024 02:01

This was referenced Oct 10, 2024

Arith to riscv lowering fastmath cmpf #3277

Merged

dialects (arith) add fastmath option for float comparisons #3272

Closed

alexarice reviewed Oct 10, 2024

View reviewed changes

alexarice assigned knickish Oct 10, 2024

alexarice added the dialects Changes on the dialects label Oct 10, 2024

alexarice requested a review from superlopuh October 10, 2024 09:15

superlopuh reviewed Oct 10, 2024

View reviewed changes

knickish requested a review from superlopuh October 12, 2024 18:28

knickish force-pushed the riscv_cmpf_fastmath branch from 52a5eaa to 7e647e4 Compare October 14, 2024 16:21

knickish changed the title ~~add fastmath flag to Li and RdRsRs Float Float Int operations~~ add fastmath flag to RdRsRs Float Float Int operations Oct 14, 2024

dialects (riscv): add fastmath flag RdRsRs Float Float Int operations…

6474cd1

…, with filecheck test

knickish force-pushed the riscv_cmpf_fastmath branch from 7e647e4 to 6474cd1 Compare October 14, 2024 16:29

superlopuh reviewed Oct 14, 2024

View reviewed changes

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

superlopuh reviewed Oct 14, 2024

View reviewed changes

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

dialects (riscv): attr_def instead of opt_prop_def

729668d

knickish force-pushed the riscv_cmpf_fastmath branch from 11cebbd to 729668d Compare October 14, 2024 23:38

superlopuh approved these changes Oct 15, 2024

View reviewed changes

superlopuh changed the title ~~add fastmath flag to RdRsRs Float Float Int operations~~ dialects: (riscv) add fastmath flag to RdRsRs Float Float Int operations Oct 15, 2024

superlopuh merged commit a7d8ebe into xdslproject:main Oct 15, 2024
14 checks passed

superlopuh added a commit that referenced this pull request Oct 23, 2024

Arith to riscv lowering fastmath cmpf (#3277)

d67b37d

`backend`(lowering)-only part of #3272. Depends on #3275 and #3276. This should close #2725 once merged Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (riscv) add fastmath flag to RdRsRs Float Float Int operations #3276

dialects: (riscv) add fastmath flag to RdRsRs Float Float Int operations #3276

knickish commented Oct 10, 2024

codecov bot commented Oct 10, 2024 •

edited

Loading

alexarice Oct 10, 2024

superlopuh Oct 10, 2024

alexarice Oct 10, 2024

knickish Oct 12, 2024

superlopuh Oct 14, 2024

superlopuh Oct 14, 2024

knickish Oct 14, 2024

superlopuh Oct 14, 2024

knickish Oct 14, 2024

alexarice Oct 10, 2024

superlopuh Oct 10, 2024

superlopuh Oct 10, 2024

knickish Oct 12, 2024

superlopuh Oct 14, 2024

knickish Oct 14, 2024

knickish commented Oct 10, 2024 •

edited

Loading

superlopuh left a comment

dialects: (riscv) add fastmath flag to RdRsRs Float Float Int operations #3276

dialects: (riscv) add fastmath flag to RdRsRs Float Float Int operations #3276

Conversation

knickish commented Oct 10, 2024

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knickish commented Oct 10, 2024 • edited Loading

superlopuh left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2024 •

edited

Loading

knickish commented Oct 10, 2024 •

edited

Loading