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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
