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

Thu Dec 1 06:08:18 PST 2022

zahiraam added inline comments.

================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:821
+        (Precision == LangOptions::ExcessPrecisionKind::FPP_Standard ||
+         Precision == LangOptions::ExcessPrecisionKind::FPP_Fast)) {
       if (Ty->isAnyComplexType()) {
----------------
rjmccall wrote:
> zahiraam wrote:
> > zahiraam wrote:
> > > rjmccall wrote:
> > > > 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.
> > > Removing the check CGF.getTarget().shouldEmitFloat16WithExcessPrecision()) here is not correct as it will perform excess precision for non-x86 architecture.  So, for now it needs to stay until we decide what needs to be done.
> > > Would it be a good alternative (may be not cheap though) to have LangOptions::useFloat16Precision() take a target as argument?
> > > 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.
> > May be in LangOptions::useFloat16Precision()? We could also have LangOptions::useBFloat16Precision and son on?
> > > 
> > > 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.
> > > 
> > Why would we want "excess precision to be specifiable for targets that don't it"? 
> > 
> > > 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.
> > 
> > Should we include some driver people to weigh in on this now? 
> > Unless you want to keep this design now and think about the other alternative later? If we do that, we will have to keep the target test in getPromotionType.  Please advise.
> > May be in LangOptions::useFloat16Precision()? We could also have LangOptions::useBFloat16Precision and son on?
> 
> Yeah, I think we want to add methods like that when we add new kinds of excess precision.
> 
> Maybe it would be helpful to think about the future directions here.  We don't need to tackle this now, but when I was doing this review, I did notice that there's a `#pragma` for controlling excess precision, and it's misimplemented: it promotes operands and thus formally changes types instead of just changing code generation.  So ultimately we're going to want to end up expressing whatever that pragma can do as part of `FPOptions` / `FPOptionsOverride`, and IRGen will be asking individual expressions whether to do them with promoted arithmetic (and what type to promote to!).
> 
> > Why would we want "excess precision to be specifiable for targets that don't it"?
> 
> Testing, or maybe reproducibility.  Mostly I don't want to preclude it.
> 
> > Should we include some driver people to weigh in on this now? 
> 
> I've asked if anyone else at Apple has a suggestion, but it would be good to reach out more broadly, yeah.  Not sure if there's a more specific person to ping than @aaron.ballman .
I renamed the option to Float16ExcessPrecision, but didn't remove the check for TargetInfo in getPromotionType (it would perform excess precision for non-x86 targets). I am wondering if useFloat16ExcessPrecision shouldn't be taking a target as argument or may be having it a virtual function of TargetInfo.

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

https://reviews.llvm.org/D136176