Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[LoongArch] Add codegen support for converting between unsigned integer and floating-point

Authored by gonglingqin on Jun 30 2022, 2:54 AM.

Diff Detail

Event Timeline

gonglingqin created this revision.Jun 30 2022, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 2:54 AM
gonglingqin requested review of this revision.Jun 30 2022, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 2:54 AM
xen0n added inline comments.Jul 5 2022, 2:00 AM

For f64 -> u32 conversion on LA64, isn't ftintrz.l.d followed by movfr2gr.s enough for all values within u32's domain? Overflow is UB both in C and LLVM IR so we can technically ignore the (very) inconsistent behavior when input overflows.

(BTW I noticed AArch64 has native support for the fptoui semantics by means of the fcvtzu insn. Hope LoongArch will gain similar niceties in a future revision...)

SixWeining added inline comments.Jul 5 2022, 2:57 AM


Actually we have considered the approach you mentioned. But similar to the divide-by-zero case, we choose the same approach as gcc what did.

And I'd like to discuss with you that is there anything missed. For example, the floating-point rounding and exception handling. ftintrz.l.d may not raise exception when the value is range [UINT32_MAX + 1, INT64_MAX - 1]. Not sure whehter we need to manually raise an exception.

@xry111 Hope to see inputs from you, too.

xen0n added inline comments.Jul 5 2022, 4:24 AM

Ah that's nice to know. Thanks for sharing the thoughts behind the current implementation decision.

As for artificial "consistency" with gcc, I think the obvious way forward is to pursue the best practice where possible, no matter which project you're working on, then make the others adopt the verified best practice. So it doesn't matter if we're working on LLVM or gcc, we simply decide on a better way then make the other project do the same.

As for the exception handling and such, my opinion is: since UB means "anything is possible", either raising exceptions or not is okay, so we may well choose the faster one, that is, simply don't care. Of course this is different from the current behavior which is saturating for out-of-range inputs, which is arguably more useful for users, but it's UB after all so I think there's definitely room for further discussion. (Ideally/academically, one would try to gather some statistics over a wide range of software corpus, then base one's argument on that; however this doesn't change the fact that such usage is invoking UB so we may not be in a position to "accommodate" for their mistakes.)

xry111 added inline comments.Jul 5 2022, 4:48 AM

In C standard, annex F specifies C language support for the IEC 60559 (aka IEEE-754) floating-point standard. F.4 para 1 says:

If the integer type is _Bool, applies and no floating-point exceptions are raised
(even for NaN). Otherwise, if the floating value is infinite or NaN or if the integral part
of the floating value exceeds the range of the integer type, then the ‘‘invalid’’ floating-
point exception is raised and the resulting value is unspecified.

So for this we need to raise an exception, unless (1) the user explicitly tells the compiler that he/she doesn't care about the exception (using -fno-trapping-math in GCC for example), or (2) LLVM explicitly claims it won't strictly follow IEEE-754 exception rules in the documentation.

xry111 added inline comments.Jul 5 2022, 4:57 AM

BUT, it seems LLVM defines fptoui w/o any exception specified. Clang's default is -fno-trapping-math (while GCC defaults to -ftrapping-math instead) and if you use -ftrapping-math explicitly, it will generate a different IR with llvm.experimental.constrained.fptoui.i32.f64. So yes: for a plain fptoui we don't need to raise exception.

gonglingqin added inline comments.Jul 6 2022, 1:53 AM

Thanks for your suggestion, after discussion we decided to change the implementation


Thanks for your suggestion, I will change it

Address @xen0n and @xry111's comments.

xen0n added inline comments.Jul 6 2022, 11:38 PM

Changes last time are lost after the rebase?

gonglingqin added inline comments.Jul 7 2022, 12:45 AM

Yes, thanks for your reminder. I'll fix this.

Address @xen0n's comments, fix lost last modification during rebase, thanks again.

xen0n accepted this revision.Jul 10 2022, 11:45 PM

Seems okay. Thanks!

This revision is now accepted and ready to land.Jul 10 2022, 11:45 PM
SixWeining accepted this revision.Jul 13 2022, 12:04 AM
This revision was landed with ongoing or failed builds.Jul 13 2022, 12:27 AM
This revision was automatically updated to reflect the committed changes.