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