Add more features #14
Conversation
ajakubowicz-canva
left a comment
ajakubowicz-canva
left a comment
There was a problem hiding this comment.
So many great improvements in this PR!!! 🎉 I haven't gotten through everything but from my first pass I think the comments I've left are the major points. Anything else will probably be a nit or can be done in a followup.
| ("min", OpSig::Binary), | ||
| ("min_precise", OpSig::Binary), | ||
| ("madd", OpSig::Ternary), | ||
| ("msub", OpSig::Ternary), |
There was a problem hiding this comment.
I don't think we need to add msub as this should be something that LLVM can optimize into.
Excerpt from a DM to Raph:
As a guiding principle, I feel like anything that can be accessed through llvm optimizations doesn't need an intrinsic API method [...] and in some cases the right thing to do might be to file issues against llvm to get more optimizations. ~@raphlinus (via DM)
There was a problem hiding this comment.
How so? Isn't this the same as for fused multiply-add, where the semantics are not exactly the same as for add + mul, and thus the compiler cannot make this optimization by default?
There was a problem hiding this comment.
I'll need Raph to chime in to answer your question because I'm also unclear about why madd isn't a similar consideration. I won't block on this.
There was a problem hiding this comment.
We discussed in office hours. I am going to try my best to summarize the discussion. Essentially, we already have a fused multiply-add, and I believe the intent is that a consumer of fearless_simd can express a fused multiply-sub by negating the fused multiply-add. Then LLVM should be able to optimize the negated fused multiply-add into the right underlying SIMD intrinsic.
This is preferred as we can keep our API surface smaller. It's also preferred because there's a complexity of which underling implementations to use on different architectures, so it's much simpler to leverage LLVM in these cases.
Finally we discussed removing msub and replacing usages with negeted fused multiply-adds.
@LaurenzV Does that summary capture the office hours discussion? Also, than you for bearing with me on this one 😄
Just clarifying that this relates to the Vello test suite right? |
Yep. |
LaurenzV
force-pushed
the
vello_cpu_part1
branch
from
11341f6 to
4a90a45
Compare
|
Just needs a fmt to pass CI. |
ajakubowicz-canva
left a comment
ajakubowicz-canva
left a comment
There was a problem hiding this comment.
After CI green LGTM.
This PR adds a bunch of additional features which already allow us to implement
in vello_cpu, using exclusively fearless_simd. The changes include:
fractandmsubmethodsTest cases pass with both, fallback and NEON.