This is an archive of the discontinued LLVM Phabricator instance.

Merge FloatingPointMode and FPEnv headers
AbandonedPublic

Authored by arsenm on Apr 1 2020, 3:31 PM.

Diff Detail

Event Timeline

arsenm created this revision.Apr 1 2020, 3:31 PM

The fp:: qualifier is kind of verbose. I'm not familiar with LLVM's C++ rules, but can we replace them with using namespace fp;? Or can we remove the fp namespace completely?

arsenm added a comment.Apr 2 2020, 5:39 AM

The fp:: qualifier is kind of verbose. I'm not familiar with LLVM's C++ rules, but can we replace them with using namespace fp;? Or can we remove the fp namespace completely?

I’m not sure, I just merged into FPEnv and followed what it was doing.

arsenm added a comment.Apr 2 2020, 7:41 AM

The fp:: qualifier is kind of verbose. I'm not familiar with LLVM's C++ rules, but can we replace them with using namespace fp;? Or can we remove the fp namespace completely?

I’m not sure, I just merged into FPEnv and followed what it was doing.

I think llvm tends to use namespaces around enums as a substitute to have qualified enum values (which I guess is a poor substitute for enum class). I would be fine eliminating the namespace and switching the existing enums to use enum class.

cameron.mcinally accepted this revision.Apr 2 2020, 8:34 AM

LGTM

That sounds good. I'm okay with doing it in this patch or leaving it for later. Your call. Everything else looks good.

This revision is now accepted and ready to land.Apr 2 2020, 8:34 AM

The fp:: qualifier is kind of verbose. I'm not familiar with LLVM's C++ rules, but can we replace them with using namespace fp;? Or can we remove the fp namespace completely?

I’m not sure, I just merged into FPEnv and followed what it was doing.

I think llvm tends to use namespaces around enums as a substitute to have qualified enum values (which I guess is a poor substitute for enum class). I would be fine eliminating the namespace and switching the existing enums to use enum class.

Using C++ strict enums looks a better way than namespaces.

Interestingly, I am working on similar patch, that unifies rounding mode enums. But I have to move the enum RondingMode to ADT/FloatingPointMode.h because the rounding mode enum is used in APFloat and it is not a good thing to refer IR header in Support library. If you consider using DenormalMode in APFloat (for example to implement constant folding dependent on this mode) you might consider keeping its definition in more common place.

arsenm added a comment.Apr 2 2020, 8:43 AM

The fp:: qualifier is kind of verbose. I'm not familiar with LLVM's C++ rules, but can we replace them with using namespace fp;? Or can we remove the fp namespace completely?

I’m not sure, I just merged into FPEnv and followed what it was doing.

I think llvm tends to use namespaces around enums as a substitute to have qualified enum values (which I guess is a poor substitute for enum class). I would be fine eliminating the namespace and switching the existing enums to use enum class.

Using C++ strict enums looks a better way than namespaces.

Interestingly, I am working on similar patch, that unifies rounding mode enums. But I have to move the enum RondingMode to ADT/FloatingPointMode.h because the rounding mode enum is used in APFloat and it is not a good thing to refer IR header in Support library. If you consider using DenormalMode in APFloat (for example to implement constant folding dependent on this mode) you might consider keeping its definition in more common place.

I think the denormal and rounding modes should be kept together, so I'm fine merging in either direction

I put my patch here: D77379. Taking rounding mode from IR/FPEnv.h may look awkward as rounding modes are used in APFloat implementation, which is in Support library. It however does not break anything as it is only header use. Moving content of IR/FPEnv.h to Support library is not an option because it contains IR specific declarations and companion source file FPEnv.cpp references things from IR library. We also could move rounding mode enumeration into APFloat.h but it would put RoundingMode and DenormalMode into different headers.

arsenm abandoned this revision.Apr 6 2020, 6:47 AM

Need to merge in the other direction, so this is backwards

clang/lib/Driver/ToolChains/PS4CPU.h