This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv][AArch64] Platform-specific builtin constrained FP enablement
ClosedPublic

Authored by kpn on Mar 30 2020, 10:16 AM.

Details

Summary

When constrained floating point is enabled the AArch64-specific builtins don't use constrained intrinsics in some cases. Fix that.

Neon is part of this patch, so ARM is affected as well.

Diff Detail

Event Timeline

kpn created this revision.Mar 30 2020, 10:16 AM
kpn marked 3 inline comments as done.Mar 30 2020, 10:18 AM

Note that the AArch64 backend isn't ready for some of these changes. That's why the test is marked XFAIL.

clang/lib/CodeGen/CGBuiltin.cpp
5706

These exit() calls tie back into my test matrix. They'll be removed when I eventually push.

clang/test/CodeGen/aarch64-neon-intrinsics-constrained.c
288

Anyone? I'm not an ARM expert.

889

???

dnsampaio added inline comments.Mar 31 2020, 4:31 AM
clang/test/CodeGen/aarch64-neon-intrinsics-constrained.c
288

The is variants of the cset instruction for both w and x. But I believe that for these tests it should be stable enough to accept only one of them.

889

It is a known issue. I believe no-one dug through the neon-emmiter machinery to find out why yet.

dnsampaio added inline comments.Apr 1 2020, 11:33 AM
clang/lib/CodeGen/CGBuiltin.cpp
8486–8492

It seems that Builder.CreateCall and Builder.CreateConstrainedFPCall usually have the same arguments, except for the function F being or not part of "experimental_constrained_".
Would it make sense to teach the Builder to select between creating a constrained or not call, depending if the function passed is constrained?

I was thinking in something like this:

Function *F = CGM.getIntrinsic( Builder.getIsFPConstrained()?
                                                        Intrinsic::experimental_constrained_fma :
                                                        Intrinsic::fma, HalfTy);
return Builder.CreateCallConstrainedFPIfRequired(F, ....
kpn marked an inline comment as done.Apr 2 2020, 6:39 AM
kpn added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
8486–8492

In CGBuiltins.cpp we already have emitUnaryMaybeConstrainedFPBuiltin() plus Binary and Ternary. They work well for the pattern seen on other hosts. But they won't easily work for this ticket.

How about a new function just below those three that will work well here? A emitCallMaybeConstrainedFPBuiltin() that takes two intrinsic IDs and chooses which one based on constrained FP would make for an even more compact use. The block of example code you put above would just turn into a single function call. Does that work for you?

dnsampaio added inline comments.Apr 2 2020, 1:58 PM
clang/lib/CodeGen/CGBuiltin.cpp
8486–8492

Sounds good to me.

kpn updated this revision to Diff 256095.Apr 8 2020, 12:55 PM

Update from review comments. The new emitCallMaybeConstrainedFPBuiltin() function does make the code much cleaner.

dnsampaio accepted this revision.Apr 9 2020, 4:18 PM

LGTM, not forgetting to remove the exit comments.

This revision is now accepted and ready to land.Apr 9 2020, 4:18 PM
This revision was automatically updated to reflect the committed changes.