This is an archive of the discontinued LLVM Phabricator instance.

[GISel]: Add Opcodes for a few Libm Intrinsics
ClosedPublic

Authored by aditya_nandakumar on Aug 7 2018, 12:12 PM.

Details

Summary

Add opcodes for llvm.intrinsic.trunc and round (only a couple for now). Opting for a verbose name with LIBM prefix for clarity.

Diff Detail

Event Timeline

I'm a little uneasy about specifying libm in the names of the intrinsics. The langref mentions libm in the description, but the semantics don't exactly match due to ignoring errno. That said, I don't really have a suggestion for a better name, so the only thing I can ask is that we make it a bit more explicit in the documentation what we mean by "LIBM" here.

include/llvm/Target/GenericOpcodes.td
516

Perhaps say "LIBM compatible intrinsics"

arsenm added a subscriber: arsenm.Aug 10 2018, 9:26 AM
arsenm added inline comments.
include/llvm/Support/TargetOpcodes.def
271–272

LIBM should definitely be removed from the names

include/llvm/Target/GenericOpcodes.td
516

Would you name it G_FTRUNC? I thought that is very similar to G_FPTRUNC and might lead to confusion. One other name I can think of is G_INTRINSIC_TRUNC which makes it explicit that this is the intrinsic equivalent in llvm IR.

arsenm added inline comments.Aug 10 2018, 10:59 AM
include/llvm/Target/GenericOpcodes.td
516

Yes

Changed the name based on feedback.

aemerson added inline comments.Aug 14 2018, 4:58 AM
include/llvm/Target/GenericOpcodes.td
516

Matt can you clarify which of those two suggestions you meant? Aditya named the intrinsics G_INTRINSIC_TRUNC but did you mean we should name them G_FTRUNC?

dsanders accepted this revision.Aug 14 2018, 3:44 PM

This LGTM. I'm ok with the G_INTRINSIC_TRUNC spelling for the trunc() intrinsic but we should reach an agreement on it before committing.

include/llvm/Target/GenericOpcodes.td
516

FWIW, I think we should make it clear in the name that it's the operation performed by trunc() (which isn't a type conversion) and not the type conversion operation performed by fptrunc from LLVM-IR.

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
1412

If we're dropping libm elsewhere we should probably drop it here too

1422

Likewise

This revision is now accepted and ready to land.Aug 14 2018, 3:44 PM
aditya_nandakumar marked 2 inline comments as done.Aug 16 2018, 2:43 PM
aditya_nandakumar added inline comments.
test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
1422

Thanks Daniel. I missed that with my search and replace. I've updated and I'll push.

aditya_nandakumar marked an inline comment as done.

Pushed in r339977