This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kpn on Jan 14 2020, 10:32 AM.

Details

Summary

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

Diff Detail

Event Timeline

kpn created this revision.Jan 14 2020, 10:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 10:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

A few comments (see inline) -- otherwise this looks good to me, thanks!

clang/lib/CodeGen/CGBuiltin.cpp
13358

Spurious empty line.

clang/test/CodeGen/builtins-systemz-vector2-constrained.c
26

Why is it that this one has metadata nodes and all the others do not? Do they really not have metadata (why?) or are you just not checking for them?

clang/test/CodeGen/builtins-systemz-zvector2-constrained.c
13

This was caused by unnecessary implicit conversions in the vecintrin.h code, I've now checked in cebba7c to remove those. Can you rebase? This FIXME should no longer be necessary.

craig.topper added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
13351

What are the semantics of vfnmadb with respect to when it rounds vs the negation? If the rounding mode is for ceil/floor and the negate is applied after, that would be different that if it was before.

13373

Same question as above.

What are the semantics of vfnmadb with respect to when it rounds vs the negation?

Hmm, interesting. The z/Architecture Principles of Operation states:

The results for each element of VECTOR FP NEGA-
TIVE MULTIPLY AND ADD and VECTOR FP NEGA-
TIVE MULTIPLY AND SUBTRACT are the same as
for VECTOR FP MULTIPLY AND ADD and VECTOR
FP MULTIPLY AND SUBTRACT, respectively, except
the sign bit of numeric results are inverted. When the
result is a NaN it’s sign bit is unchanged.

So as far as rounding is concerned, the vfnmadb should have the exact same result as a vfmadb ; vflcdb sequence we'd get from a fma/fneg.

I hadn't realized the NaN special treatment before. This makes the result actually different in that case. OTOH I guess IEEE only says the result has to some NaN, not particularly which NaN, so that's probably also OK. (In any case it's unrelated to strict FP.)

kpn marked an inline comment as done.Jan 21 2020, 7:47 AM
kpn added inline comments.
clang/test/CodeGen/builtins-systemz-vector2-constrained.c
26

No reason, really. The regular expression will pick up the second metadata argument if present, so I could just eliminate the check for the second one here. This isn't a metadata test so precise testing of metadata arguments doesn't seem necessary? Consistency would be good, though.

kpn updated this revision to Diff 239324.Jan 21 2020, 7:48 AM

Update for review comments.

kpn updated this revision to Diff 239325.Jan 21 2020, 7:50 AM

Eliminate a blank line I thought I had already had gotten.

kpn marked 2 inline comments as done.Jan 21 2020, 7:51 AM
uweigand accepted this revision.Jan 21 2020, 8:47 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jan 21 2020, 8:47 AM
This revision was automatically updated to reflect the committed changes.