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