aaron.ballman added a reviewer: MaskRay.
aaron.ballman added a comment.
Herald added a subscriber: StephenFan.
Adding @MaskRay for driver-specific expertise.
================
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:
> > 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.
>
>
> 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've roped in @MaskRay as code owner for questions about the driver behavior.
> 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.
I think it might make more sense to put the interface on the `Type` instead of
on `TargetInfo`; so long as it accepts an `ASTContext` object, you can get back
to language options and target information. e.g., `bool
Type::usesExcessPrecision(const ASTContext &Ctx) const;` so that we wind up
with a single interface instead of having to add a new interface for every type
which has excess precision. WDYT?
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
