rjmccall added inline comments.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:821
+ (Precision == LangOptions::ExcessPrecisionKind::FPP_Standard ||
+ Precision == LangOptions::ExcessPrecisionKind::FPP_Fast)) {
if (Ty->isAnyComplexType()) {
----------------
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 .
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
