mibintc marked 2 inline comments as done.
mibintc added inline comments.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:227
+ FMF.setAllowContract(FPFeatures.allowFPContractAcrossStatement() ||
+ FPFeatures.allowFPContractWithinStatement());
}
----------------
mibintc wrote:
> michele.scandale wrote:
> > I'm not convinced it correct to set `contract` when
> > `allowFPContractWithinStatement` return true. Can someone clarify this?
> >
> > If I compile the following example with `-ffp-contract=on`:
> > ```
> > float test1(float a, float b, float c) {
> > float x = a * b;
> > return x + c;
> > }
> >
> > float test2(float a, float b, float c) {
> > return a * b + c;
> > }
> > ```
> >
> > Before this change the generated code was:
> > ```
> > define float @test1(float %a, float %b, float %c) {
> > %0 = fmul float %a, %b
> > %1 = fadd float %0, %c
> > ret float %1
> > }
> >
> > define float @test2(float %a, float %b, float %c) {
> > %0 = call float @llvm.fmuladd.f32(float %a, float%b, float %c)
> > ret float %0
> > }
> > ```
> >
> > And my understanding is that the in-statement contraction is implemented by
> > emitting the `llvm.fmuladd` call that a backend might decide to implement
> > as `fmul + fadd` or as `fma`.
> >
> > With this change the generated code is:
> > ```
> > define float @test1(float %a, float %b, float %c) {
> > %0 = fmul contract float %a, %b
> > %1 = fadd contract float %0, %c
> > ret float %1
> > }
> >
> > define float @test2(float %a, float %b, float %c) {
> > %0 = call contract float @llvm.fmuladd.f32(float %a, float%b, float %c)
> > ret float %0
> > }
> > ```
> > and it seems to me that in `test1` (where multiple statements where
> > explicitly used) the optimizer is now allowed to perform the contraction,
> > violating the original program semantic where only "in-statement"
> > contraction was allowed.
> Thanks @michele.scandale i will work on a patch for this
@michele.scandale I posted a patch for 'contract' here,
https://reviews.llvm.org/D79903
================
Comment at: clang/lib/Serialization/ASTReader.cpp:7899
+ if (FpPragmaCurrentLocation.isInvalid()) {
+ assert(*FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue &&
+ "Expected a default pragma float_control value");
----------------
yaxunl wrote:
> mibintc wrote:
> > yaxunl wrote:
> > > This changes the behavior regarding AST reader and seems to be too hash
> > > restriction. Essentially this requires a pch can only be used with the
> > > same fp options with which the pch is generated. Since there are lots of
> > > fp options, it is impractical to generate pch for all the combinations.
> > >
> > > We have seen regressions due to this assertion.
> > >
> > > Can this assertion be dropped or done under some options?
> > >
> > > Thanks.
> > >
> > @yaxunl Can you please send me a reproducer, I'd like to see what's going
> > on, not sure if just getting rid of the assertion will give the desired
> > outcome.
> {F11915161}
>
> Pls apply the patch.
>
> Thanks.
@rjmccall In the example supplied by @yaxunl, the floating point options in the
pch file when created are default, and the floating point options in the use
have no-signed-zeros flag. The discrepancy causes an error diagnostic when the
pch is used. I added the FMF flags into FPFeatures in this patch, I made them
COMPATIBLE_LANGOPT which is the encoding also being used for FastMath,
FiniteMathOnly, and UnsafeFPMath. Do you have some advice about this issue?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72841/new/
https://reviews.llvm.org/D72841
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
