[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 };
+
----------------
zahiraam wrote:
> rjmccall wrote:
> > 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.
> Not sure I understand this comment.  The enum is already named ExcessPrecisionKind!
> I have added a cc1 option at line 1580 of Options.td called ffloat16-excess-precision whose type is ExcessPrecisionKind .
> Am I misunderstanding something?
You've named the language option ExcessPrecision, implying it's not Float16-specific.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:278
   QualType getPromotionType(QualType Ty) {
     if (CGF.getTarget().shouldEmitFloat16WithExcessPrecision()) {
       if (Ty->isRealFloatingType()) {
----------------
You can remove the target check here.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:821
+        (Precision == LangOptions::ExcessPrecisionKind::FPP_Standard ||
+         Precision == LangOptions::ExcessPrecisionKind::FPP_Fast)) {
       if (Ty->isAnyComplexType()) {
----------------
rjmccall wrote:
> 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.
Sorry, I'm waffling about the right way to handle this.  Let me lay out what I'm thinking.

1. It's best if, by the time we get into the main compiler operation, there's a single place we can check to ask if a particular type should use excess precision.  This code can actually be relatively hot, so we want it to be cheap to check, and for correctness we want it to be a simple condition.

2. I don't like the design of `-fexcess-precision`.  It mashes the handling of all of the types together, and I'd like excess precision for different types to be independently controllable.  In principle, I'd even like excess precision to be specifiable when the target doesn't need it.  It makes sense for the driver to worry about all these poorly-designed options and just give precise controls to the frontend.

3. The problem with that is that the driver doesn't have all the information it would need in order to pick the right default.  Or, well, it has the information, but it would have to parse it out of the command line in a way that we currently try to avoid in the driver.  For example, to pick the default for `_Float16`, we need to know if AVX512FP16 is enabled in the target, and as far as I know, the first time that anything knows that for sure is after we construct a TargetInfo object in CompilerInstance.

4. So I'm leaning back towards the idea that we should just pass `-fexcess-precision` down to the frontend instead of processing it in the driver, and then the frontend can reconcile that option with the precise target info and turn it into a bunch of derived, type-specific language options.  Most of the compiler will at least still be able to consider only those type-specific language options.

But I'd like to get some driver experts to think about it.


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

https://reviews.llvm.org/D136176