Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dialects: (riscv_snitch) add fastmath flags in vector ops #2913

Merged
merged 3 commits into from
Jul 21, 2024

Conversation

superlopuh
Copy link
Member

It feels like fastmath is still appropriate on the vector ops, also it will make the lowering a little less tedious, by making the vector and scalar arithmetic ops a little more uniform.

@superlopuh superlopuh added the dialects Changes on the dialects label Jul 21, 2024
@superlopuh superlopuh self-assigned this Jul 21, 2024
Copy link

codecov bot commented Jul 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.90%. Comparing base (22b8564) to head (877571d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2913   +/-   ##
=======================================
  Coverage   89.90%   89.90%           
=======================================
  Files         403      403           
  Lines       50424    50424           
  Branches     7788     7788           
=======================================
  Hits        45334    45334           
  Misses       3862     3862           
  Partials     1228     1228           

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

Copy link
Contributor

@zero9178 zero9178 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do RISC-V operations have fast math actually? I'd have thought that fast math would be gone past instruction selection (aka arith to riscv lowering).

Does this enable anything that cannot be done on arith?

Base automatically changed from sasha/memref_stream/ssum-init to main July 21, 2024 16:38
@superlopuh
Copy link
Member Author

fmadd

@zero9178
Copy link
Contributor

fmadd

I'd have expected this to be implemented within arith-to-riscv as a simple DAG match. Having fastmath feels wrong to me because suddenly the semantics of instructions that have fast math do not match exactly the ones of the RISC-V ISA anymore which feels a bit odd.

Shouldn't be a correctness issue though I guess, so if this is the state of the world then so be it.

@superlopuh
Copy link
Member Author

My understanding is that this follows the behaviour of LLVM, although we might want to diverge from it. Also, isn't that the definition of fastmath, to diverge from the semantics?

@superlopuh superlopuh merged commit e863366 into main Jul 21, 2024
10 checks passed
@superlopuh superlopuh deleted the sasha/memref_stream/ssum-fastmath branch July 21, 2024 17:39
@tobiasgrosser
Copy link
Contributor

My understanding is that this follows the behaviour of LLVM, although we might want to diverge from it. Also, isn't that the definition of fastmath, to diverge from the semantics?

Can you clarify how this 'follows the behaviour of LLVM'. I am not sure what you are precisely referring to. It might indeed be interesting to investigate how LLVM implements fastmath. Typically fastmath flags are at the LLVM-IR level, but I am unsure if it also maintains them in the backend. It may or may not, and I would find it useful to understand what LLVM does. In general, we should follow LLVM except if there good reasons to do sth different.

@superlopuh
Copy link
Member Author

What I mean is that LLVM did emit fmadd instructions in our flow without the fastmath flags. I think @compor initially added the flags to our experimental setup and set up the corresponding infrastructure in xDSL to make sure that we also emit fmadd instructions when explicitly specified by the user.
https://discourse.llvm.org/t/risc-v-multiply-add-instructions-fmadd-etc-bit-exactness-and-correctness-of-optimizations/64194
In this discussion, the reference is the C standard. I don't know what the specification of arith on floats is but it seems to mostly follow the C standard/clang/llvm approach of only change the fp operations when the relevant flags are set, and not by default.

@tobiasgrosser
Copy link
Contributor

Are you saying for C-code that was compiled with clang, fmadds were created. Or that LLVM-IR that you generated without fastmath flags was fused by LLVM (or later in its backend). The link you posted suggests that only the former is legal.

On the other hand, @nazavode's research seems to suggest that LLVM indeed maintains fastmath flags up until the very end.

@superlopuh
Copy link
Member Author

What I'm saying is with the flags, fmadds were created by the backend, and without they were not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants