This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv] clang support for constrained FP builtins
ClosedPublic

Authored by kpn on Nov 14 2019, 10:24 AM.

Details

Summary

Currently if constrained fp math is requested clang is still emitting calls to the non-constrained versions of math builtins. This patch corrects that for calls that are not target-specific.

I really wanted to keep constrained/non-constrained decisions in the IRBuilder, but that causes complications. This version stays close to that ideal without the complications.

Diff Detail

Event Timeline

kpn created this revision.Nov 14 2019, 10:24 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 14 2019, 10:24 AM
rjmccall added inline comments.Nov 15 2019, 1:17 PM
clang/lib/CodeGen/CGBuiltin.cpp
357

I don't see any situation where not_intrinsic is passed in here; why all these checks?

2297

What can this not use the standard path?

kpn marked 2 inline comments as done.Nov 18 2019, 1:15 PM
kpn added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
357

I tend to be conservative, and I'm not good at predicting the future. I can remove the checks easily in the next round.

2297

I'll take another look. It wasn't using the standard path before so I made my added code match. If it can use the standard path in both cases then maybe it should?

rjmccall added inline comments.Nov 18 2019, 1:25 PM
clang/lib/CodeGen/CGBuiltin.cpp
357

I just can't imagine why you would ever call this function without a constrained intrinsic ID when its only purpose is to choose between a constrained and unconstrained intrinsic. It's always better to start with stronger preconditions and then generalize later if necessary.

2297

Yeah. It's possible the type-heterogeneity (the power always being an int) stops that from working for some reason, but if not, it's always nicer to re-use more code.

kpn updated this revision to Diff 233073.Dec 10 2019, 6:11 AM

Update for review comments.

rjmccall accepted this revision.Dec 10 2019, 9:51 AM

Thanks, LGTM.

This revision is now accepted and ready to land.Dec 10 2019, 9:51 AM
This revision was automatically updated to reflect the committed changes.