[builtins] Start to refactor int to fp conversion functions to use a common implementation #66903
Conversation
asb
changed the title
asb
force-pushed
the
2023q3-compiler-rt-refactor-int-to-fp-1
branch
2 times, most recently
from
9730c2e
to
324ecfb
Compare
|
Ping. See #67540 for the next patch in the series. |
|
EDIT: I'd erroneously said that clang generates identical IR - this isn't true. However, alive2 does seem to show that the functions are equivalent (not totally clear to me this is valid usage). floatdidf old vs new: https://alive2.llvm.org/ce/z/XTadv9 floatundidf old vs new: https://alive2.llvm.org/ce/z/zY7Txo |
|
Ping? See above comment for Alive's successful comparison of old vs new IR. |
There was a problem hiding this comment.
LGTM - Note that I'm only LGTMing this because of the alive2 proof Alex mentioned above. I wouldn't be comfortable approving this (correctness wise) otherwise. Given we know it's correct, all that's left is the stylistic question and this looks entirely reasonable to me. I'm definitely out of area though, so if someone more active in this area has an opinion, please speak up.
…common implementation After this patch, the softfp implementations of floatdidf and floatundidf use a common implementation (int_to_fp.h and int_to_fp_impl.inc). This roughly follows the pattern used for a wide range of other builtins, e.g. fp_trunc_impl.inc. Currently there is substantial copy and paste for the various int to fp conversion functions, with just a few constants being changed. This is a barrier to maintainability, and it's also not attractive to copy this approach as we introduce additional int to fp conversion functions for bf16 and half (which we currently lack, but need - see <https://reviews.llvm.org/D157509>). I've opted to conservatively start by replacing just two functions, leaving a follow-up patch to replace others that follow the same pattern. Also, for better or worse I've left the logic in float[un]didf largely unchanged other than using a similar approach to fp_trunc_impl.inc to remove the constants that are tied to a specific output floating point format.
asb
force-pushed
the
2023q3-compiler-rt-refactor-int-to-fp-1
branch
from
324ecfb
to
475a67d
Compare
Builds in llvm#66903, converting the rest of the low-hanging fruit to use the common implementation.
…ion (#67540) Builds on #66903, converting the rest of the low-hanging fruit to use the common implementation. See #67540 (comment) for links to Alive2 comparisons of before/after.
|
This seems to have broken the build under MSVC: |
MSVC in C mode apparently doesn't consider `const int` to be sufficiently constant expression This was broken in #66903.
Thanks for fixing, I'm sorry I'd missed this - seems I had an issue with my email filter rules. I'm really surprised the code in int_to_fp.h was problematic but fp_trunc_impl.inc isn't. |
Thank you and apologies for breaking your build, commented there with what I think the problem is. |
After this patch, the softfp implementations of floatdidf and floatundidf use a common implementation (int_to_fp.h and int_to_fp_impl.inc). This roughly follows the pattern used for a wide range of other builtins, e.g. fp_trunc_impl.inc.
Currently there is substantial copy and paste for the various int to fp conversion functions, with just a few constants being changed. This is a barrier to maintainability, and it's also not attractive to copy this approach as we introduce additional int to fp conversion functions for bf16 and half (which we currently lack, but need - see https://reviews.llvm.org/D157509).
I welcome feedback on the approach taken here, as well as the best way to move forward to land it. I've opted to conservatively start by replacing just two functions, leaving a follow-up patch to replace others that follow the same pattern. Also, for better or worse I've left the logic in float[un]didf largely unchanged other than using a similar approach to fp_trunc_impl.inc to remove the constants that are tied to a specific output floating point format.
For something like this I really miss the stacked reviews we had on Phabricator, but getting some feedback on this rough first patch is probably sensible before pushing out a longer series.