This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Handle llvm.roundeven
ClosedPublic

Authored by arsenm on Jul 20 2020, 5:31 AM.

Details

Summary

I still think it's highly questionable that we have two intrinsics
with identical behavior and only vary by the name of the libcall used
if it happens to be lowered that way, but try to reduce the feature
delta between SDAG and GlobalISel for recently added intrinsics. I'm
not sure which opcode should be considered the canonical one, but
lower roundeven back to round.

Diff Detail

Event Timeline

arsenm created this revision.Jul 20 2020, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2020, 5:31 AM
sepavloff added inline comments.Jul 21 2020, 12:34 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2494

I am afraid this change breaks the semantics of roundeven. Intrinsic round implements the same operation as libm function round (http://llvm.org/docs/LangRef.html#id554). In the latest draft (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2478.pdf, 7.12.9.6p2) the function round is described:

The round functions round their argument to the nearest integer value in floating-point format,
rounding halfway cases away from zero, regardless of the current rounding direction.

In the same draft roundeven is described (7.12.9.8p2):

The roundeven functions round their argument to the nearest integer value in floating-point format,
rounding halfway cases to even (that is, to the nearest value that is an even integer), regardless of
the current rounding direction.

round and roundeven implement different rounding modes. Both functions do not depend on the current rounding mode.

LIBC variants provide roundeven but these implementations look complicated. There must be an algorithm which uses trunc and implements roundeven suitable for vector operations. Probably the algorithm that uses remainder in https://stackoverflow.com/questions/32746523/ieee-754-compliant-round-half-to-even can be used to implement roundeven similar to round in LegalizerHelper::lowerIntrinsicRound.

paquette added inline comments.Jul 21 2020, 2:32 PM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2493

typo: operatio

arsenm marked an inline comment as done.Jul 21 2020, 3:04 PM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2494

Oh right, this should have been G_FRINT. I can't keep all the rounding functions straight

arsenm updated this revision to Diff 279646.Jul 21 2020, 3:46 PM

Expand to G_FRINT

sepavloff added inline comments.Jul 23 2020, 6:40 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2494

Why not adding new target opcode, like G_FROUNDEVEN or G_INTRINSIC_ROUNDEVEN?

arsenm marked an inline comment as done.Jul 23 2020, 1:02 PM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2494

That is added, and this is where it's lowered to the other equivalent operation

sepavloff added inline comments.Jul 27 2020, 9:17 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2494

Indeed :)
What is the purpose of adding such replacement? In general, it is invalid, as it may be applied only in default FP mode. Using library calls is a safer solution. IIUC, if some target is required to support roundeven it must either implement custom lowering or appropriate libc must be used. Are there any reasons to allow such incomplete solution?

arsenm marked an inline comment as done.Jul 27 2020, 12:46 PM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
2494

AMDGPU has no library calls and does have the instruction. The unconstrained FP operations are defined as running in the default FP mode. It's quite undefined to execute them in another mode, these are equivalent operations.

sepavloff accepted this revision.Jul 28 2020, 12:04 AM

The patch looks good to me, however I am not experienced in GlobalISel. Someone more familiar with it should look at this patch.

This revision is now accepted and ready to land.Jul 28 2020, 12:04 AM