dialects: (llvm) Add a bunch of float methods #3781
Conversation
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
from
47272fd to
73d8169
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3781 +/- ##
==========================================
- Coverage 91.23% 91.23% -0.01%
==========================================
Files 461 461
Lines 57445 57480 +35
Branches 5546 5547 +1
==========================================
+ Hits 52411 52441 +30
- Misses 3612 3616 +4
- Partials 1422 1423 +1 ☔ View full report in Codecov by Sentry. |
|
Do we have an mlir integration test for llvm ops? If not I feel it would be useful. |
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
4 times, most recently
from
3a9d650 to
7c83cf7
Compare
Added! |
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
2 times, most recently
from
55813c5 to
bf4562f
Compare
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
4 times, most recently
from
bbec3c8 to
f9e9a99
Compare
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
2 times, most recently
from
117e9eb to
d018456
Compare
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
from
d018456 to
f1c4442
Compare
But it doesn't. And fixing the assembly format is outside the scope of this PR. I'm not able to fix the assembly format today. |
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
3 times, most recently
from
cb55182 to
8a3c409
Compare
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
from
8a3c409 to
774aee4
Compare
AntonLydike
changed the base branch from
main
to
alexarice/default-prop-in-dict
|
Rebased on top of #3783, @alexarice can you okay now? |
|
|
||
|
|
||
| @irdl_op_definition | ||
| class SIToFPOp(GenericCastOp): |
There was a problem hiding this comment.
These are currently very unconstrained but this isn't technically wrong
There was a problem hiding this comment.
I think it kinda is, you can't cast from a float with this. Added a verify check.
There was a problem hiding this comment.
The custom verifier is verify_
There was a problem hiding this comment.
Damn, LinkageAttr also uses the wrong verifier then. Just copied from there as I'm never sure which one to use 🙃
There was a problem hiding this comment.
It's different for attributes
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
from
774aee4 to
f256fae
Compare
|
|
||
|
|
||
| @irdl_op_definition | ||
| class SIToFPOp(GenericCastOp): |
There was a problem hiding this comment.
The custom verifier is verify_
xdsl/dialects/llvm.py
Outdated
| if not isinstance(self.arg.type, IntegerType): | ||
| raise VerifyException("Expected integer type as input") | ||
| if not isinstance(self.result.type, AnyFloat): | ||
| raise VerifyException("Expected output to be a float") |
There was a problem hiding this comment.
I don't really understand why you would check input and output types in a custom verifier
There was a problem hiding this comment.
because then I can subclass the main conversion thingy class and keep the remaining bits.
There was a problem hiding this comment.
The only shared functionality seems to be the assembly format
|
Could you stop resolving discussions please, makes it hard to check what's happened |
|
|
||
| fastmathFlags = prop_def(FastMathAttr, default_value=FastMathAttr(None)) | ||
|
|
||
| traits = traits_def(Pure()) |
There was a problem hiding this comment.
Could you please use theSameOperandsAndResultType trait where applicable?
It shouldn't be here, as not all ops that inherit from this base should comply to it.
| // float arith: | ||
|
|
||
| %fmul = llvm.fmul %f1, %f1 : f32 | ||
| // CHECK: %{{\d+}} = llvm.fmul %2, %2 : f32 |
There was a problem hiding this comment.
Any reason for not using a pattern capture here (i.e., {{.*}} instead of %2)?
There was a problem hiding this comment.
hmm, why are they not %f1 in the first place?
There was a problem hiding this comment.
Likely better to use a numeric capture if you really want to avoid hard coding this
| // CHECK: %{{\d+}} = llvm.fmul %2, %2 : f32 | |
| // CHECK: %{{\d+}} = llvm.fmul %[[#in:]], %[[#in]] : f32 |
There was a problem hiding this comment.
yes, that's what I meant :)
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
from
f256fae to
539ee08
Compare
AntonLydike
force-pushed
the
anton/llvm-float-stuff
branch
from
539ee08 to
861f44c
Compare
Adds the following llvm floating point operations: - `llvm.fadd` - `llvm.fsub` - `llvm.fmul` - `llvm.fdiv` - `llvm.frem` - `llvm.sitofp` - `llvm.fpext` Closes xdslproject#3780 I also took the liberty to sort the dialect lists alphabetically
Adds the following llvm floating point operations:
llvm.faddllvm.fsubllvm.fmulllvm.fdivllvm.fremllvm.sitofpllvm.fpextCloses #3780
I also took the liberty to sort the dialect lists alphabetically