[PATCH] D136176: Implement support for option 'fexcess-precision'.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.h:301
 
+  enum Float16ExcessPrecisionKind { FPP_Standard, FPP_Fast, FPP_None };
+
----------------
rjmccall wrote:
> You can leave this named `ExcessPrecisionKind` — if we introduce excess precision for other types, they'll have the same set of options.
There was a miscommunication here.  Please leave this enum named `ExcessPrecisionKind`, since its values could be applied to any FP type.  However, please set up the language options so that different types are independently controlled.  So there should be a option called `Float16ExcessPrecision` whose type is `ExcessPrecisionKind`. If we add a similar option for `__bf16`, it will be called `BF16ExcessPrecision` and will also have type `ExcessPrecisionKind`, and so on.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:821
+        (Precision == LangOptions::ExcessPrecisionKind::FPP_Standard ||
+         Precision == LangOptions::ExcessPrecisionKind::FPP_Fast)) {
       if (Ty->isAnyComplexType()) {
----------------
Let's make the language option be canonically correct: if the target doesn't want us to emit `_Float16` with excess precision, we should either diagnose or ignore the frontend option, but in either case clients like this should be able to just look at the LangOpt.  We should do this in the frontend, not the driver.

Also, we have a similar function in the complex emitter, right?

To allow for multiple types with independent excess precision in the future, please sink the checks down to where we've recognized that we're dealing with a certain type, like:

```
if (auto *CT = Ty->getAs<ComplexType>()) {
  QualType ElementType = CT->getElementType();
  if (ElementType->isFloat16Type() &&
      CGF.getContext().getLangOpts().getFloat16ExcessPrecision() != LangOptions::ExcessPrecisionKind::FPP_None)
    return CGF.getContext().getComplexType(CGF.getContext().FloatTy);
}
```

You might also consider adding a `useFloat16ExcessPrecision()` convenience function to LangOpts for this.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2997
+        D.Diag(diag::err_drv_unsupported_opt_for_target)
+            << A->getAsString(Args) << TC.getTriple().str();
+      if (Val.equals("standard") || Val.equals("fast")) {
----------------
This logic looks weird.  You're emitting an error about "16", but only on x86 and x86_64?  Shouldn't this be inverted?

I think the right way to understand this option is that it's totally target-specific what types this applies to.  Consider breaking it down by target first, something like:

```
// On 32-bit and 64-bit x86, interpret this as just controlling _Float16, and
// accept "16" as disabling excess precision.
if (Arch == llvm::Triple::x86 ||
    Arch == llvm::Triple::x86_64) {
  if (Val.equals("standard") || Val.equals("fast"))
    Float16ExcessPrecision = Val;
  if (Val.equals("16"))
    Float16ExcessPrecision = "none";
  else
    D.Diag(diag::err_drv_unsupported_option_argument)
      << A->getSpelling() << Val;
// On other targets, accept but ignore "standard" and "fast".
} else {
  if (!(Val.equals("standard") || Val.equals("fast")))
    D.Diag(diag::err_drv_unsupported_option_argument)
      << A->getSpelling() << Val;
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136176/new/

https://reviews.llvm.org/D136176