Page MenuHomePhabricator

Move floating point related entities to namespace level
ClosedPublic

Authored by sepavloff on Oct 29 2019, 12:26 AM.

Details

Summary

Enumerations that describe rounding mode and exception behavior were
defined inside ConstrainedFPIntrinsic. It makes sense to use the same
definitions to represent the same properties in other cases, not only
in constrained intrinsics. It was however inconvenient as required to
include constrained intrinsics definitions even if they were not needed.
Also using long scope prefix reduced readability.

This change moves these definitioins to the namespace llvm.
No functional changes.

Diff Detail

Event Timeline

sepavloff created this revision.Oct 29 2019, 12:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2019, 12:26 AM

I like this. I have just one minor suggestion.

llvm/include/llvm/IR/FloatingPoint.h
29 ↗(On Diff #226846)

What would you think of adding a wrapping namespace to give these more context? So instead of "llvm::RoundingMode" we'd have "llvm::fp::RoundingMode" or, more commonly "fp::RoundingMode".

simoll added a subscriber: simoll.Nov 6 2019, 1:09 AM

Thanks for the patch!
Factoring the constrained fp enums out of the ConstrainedFPIntrinsic is also something we need to the vector predication extension (https://reviews.llvm.org/D57504).
One nitpick, the name FloatingPoint.h seems very unspecific. How about going for something like FPEnv.h?

rkruppe added a subscriber: rkruppe.Nov 6 2019, 1:22 AM
sepavloff updated this revision to Diff 228046.Nov 6 2019, 5:47 AM

Updated patch according to reviewers notes

  • Renamed files FloatingPoint.{h,cpp} to FEnv.{h,cpp}.
  • Enumerations RoundingMode and ExceptionBehavior are moved to namespace llvm::fp.
arsenm added a subscriber: arsenm.Nov 6 2019, 8:50 AM

I think these enums should go into the same header as added in D69598

llvm/unittests/IR/IRBuilderTest.cpp
252–254

These should all use EXPECT_EQ

sepavloff updated this revision to Diff 228174.Nov 6 2019, 8:51 PM
sepavloff marked an inline comment as done.

Updated patch according to reviewers notes

  • Used ASSERT_EQ instead of ASSERT_TRUE in unit tests.
sepavloff marked an inline comment as done.Nov 6 2019, 9:19 PM

I think these enums should go into the same header as added in D69598

It is definitely a good idea. But I would propose to move enums from llvm/ADT/FloatingPointMode.h in D69598 to the file FPEnv.h from this patch. The directory ADT is for Advanced Data Types and contains mostly general purpose classes. On the other hand enum DenormalMode in that file implements representation of floating point environment. Now it is used in CodeGen, but exposing it to users seems to be natural step, as now we do not have a way to control handling denormals.

We need to agree upon file name. Initially the file was FloatingPoint.h, then it was renamed to FPEnv.h to be more specific. What is your opinion about the name?
Also we could agree whether to use namespaces or strict enums.

arsenm added inline comments.Nov 6 2019, 9:54 PM
llvm/unittests/IR/IRBuilderTest.cpp
290

EXPECT_EQ, not assert. The argument order should also be swapped

sepavloff updated this revision to Diff 228181.Nov 6 2019, 11:26 PM

Updated patch according to reviewer's notes

  • Replaced ASSERT_EQ with EXPECT_EQ in unit test.
mibintc added a subscriber: mibintc.Nov 7 2019, 1:05 PM
arsenm added a comment.Nov 7 2019, 5:54 PM

I think these enums should go into the same header as added in D69598

It is definitely a good idea. But I would propose to move enums from llvm/ADT/FloatingPointMode.h in D69598 to the file FPEnv.h from this patch. The directory ADT is for Advanced Data Types and contains mostly general purpose classes. On the other hand enum DenormalMode in that file implements representation of floating point environment. Now it is used in CodeGen, but exposing it to users seems to be natural step, as now we do not have a way to control handling denormals.

We need to agree upon file name. Initially the file was FloatingPoint.h, then it was renamed to FPEnv.h to be more specific. What is your opinion about the name?
Also we could agree whether to use namespaces or strict enums.

ADT is poorly named and has other things along the lines of generally useful enums. I don't really care whether this goes in IR or ADT or Support

andrew.w.kaylor accepted this revision.Nov 14 2019, 11:55 AM

This looks good to me. I like the header file name and location you have here.

This revision is now accepted and ready to land.Nov 14 2019, 11:55 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 15, 4:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript