[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.
Herald added subscribers: jansvoboda11, dexonsmith.

I got a bug report about this patch, see 
https://bugs.llvm.org/show_bug.cgi?id=49479.  I put a patch to fix it here, 
https://reviews.llvm.org/D98211


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Changpeng Fang via Phabricator via cfe-commits]

cfang added a comment.

-ffast-math flag got lost in the Builder after this change.

FMF.isFast() is true before  updateFastMathFlags(FMF, FPFeatures), but turns 
false after. 
It seems the Builder.FMF has been correctly set before, but I am not clear what 
FPFeatures should be at this point:

+static void setBuilderFlagsFromFPFeatures(CGBuilderTy ,
+  CodeGenFunction ,
+  FPOptions FPFeatures) {
+  auto NewRoundingBehavior = FPFeatures.getRoundingMode();
+  Builder.setDefaultConstrainedRounding(NewRoundingBehavior);
+  auto NewExceptionBehavior =
+  ToConstrainedExceptMD(FPFeatures.getExceptionMode());
+  Builder.setDefaultConstrainedExcept(NewExceptionBehavior);
+  auto FMF = Builder.getFastMathFlags();
+  updateFastMathFlags(FMF, FPFeatures);
+  Builder.setFastMathFlags(FMF);
+  assert((CGF.CurFuncDecl == nullptr || Builder.getIsFPConstrained() ||
+  isa(CGF.CurFuncDecl) ||
+  isa(CGF.CurFuncDecl) ||
+  (NewExceptionBehavior == llvm::fp::ebIgnore &&
+   NewRoundingBehavior == llvm::RoundingMode::NearestTiesToEven)) &&
+ "FPConstrained should be enabled on entire function");
+}


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.
Herald added a subscriber: sstefan1.

I documented the issue reported by @yaxunl here, 
https://bugs.llvm.org/show_bug.cgi?id=46166, and take ownership of the bug. 
Thanks for the report.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:227
+  FMF.setAllowContract(FPFeatures.allowFPContractAcrossStatement() ||
+   FPFeatures.allowFPContractWithinStatement());
 }

mibintc wrote:
> 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 
Thanks!


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:7899
+if (FpPragmaCurrentLocation.isInvalid()) {
+  assert(*FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue &&
+ "Expected a default pragma float_control value");

mibintc wrote:
> 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?  
A couple things are going on here.

First: a PCH can only end at the top level, not in the middle of a declaration, 
but otherwise Sema can be in an arbitrary semantic configuration.  That 
definitely includes arbitrary pragmas being in effect, so in general the end 
state might not match the default FP state, so this assertion is bogus.  When 
loading a PCH, you need to restore the pragma stack and current FP state to the 
configuration it was in at the end of the PCH.

Second: if you restore the pragma stack and FP state naively given the current 
representation of FP state, you will completely overwrite the FP settings of 
the current translation unit with the FP settings that were in effect when the 
PCH was built, which is obviously not okay.  This is one way (among several) 
that the current representation is not really living up to the statement that 
these language options are "compatible".  The better way to do this would be 
for the pragma stack and Expr nodes to record the current set of overrides in 
effect rather than the absolute current state; this could then be easily 
applied to an arbitrary global FP state.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:7899
+if (FpPragmaCurrentLocation.isInvalid()) {
+  assert(*FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue &&
+ "Expected a default pragma float_control value");

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.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked an inline comment as done.
mibintc added inline comments.



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:
> 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. 


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:7899
+if (FpPragmaCurrentLocation.isInvalid()) {
+  assert(*FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue &&
+ "Expected a default pragma float_control value");

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.



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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:227
+  FMF.setAllowContract(FPFeatures.allowFPContractAcrossStatement() ||
+   FPFeatures.allowFPContractWithinStatement());
 }

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


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Steve Canon via Phabricator via cfe-commits]

scanon added a comment.

> Prior to this change contract was never generated in the case of in-statement 
> contraction only, instead clang was emitting llvm.fmuladd to inform the 
> backend that only those were eligible for contraction. From a correctness 
> perspective I think this was perfectly fine.
> 
> Currently I don't see any logic to generate "blocking intrinsics" (I guess to 
> define a region around the instructions emitted for the given statement). 
> Until such mechanism is in place, I think that generating the contract 
> fast-math flag also for in-statement contraction is wrong because it breaks 
> the original program semantic.

This is exactly right. If we are going to have new in-statement optimizations, 
then we probably do need to add some blocking intrinsic (which would be 
elidable given suitable fast-math flags); the system of generating fmuladd 
works well for FMA contraction, but doesn't really generalize to other 
optimizations of this sort.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

No, if that's how we handle that, then you're absolutely right that we 
shouldn't set the `contract` flag.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added a comment.

In D72841#2030707 , @rjmccall wrote:

> IIUC, the way within-statement contraction is supposed to work is that there 
> are supposed to be blocking intrinsics inserted at various places.  I don't 
> remember the details, though, or if anyone's thought about how it interacts 
> with cross-statement contraction, which this pragma permits within the same 
> function (and could occur with inlining regardless).


Prior to this change `contract` was never generated in the case of in-statement 
contraction only, instead clang was emitting `llvm.fmuladd` to inform the 
backend that only those were eligible for contraction. From a correctness 
perspective I think this was perfectly fine.

Currently I don't see any logic to generate "blocking intrinsics" (I guess to 
define a region around the instructions emitted for the given statement). Until 
such mechanism is in place, I think that generating the `contract` fast-math 
flag  also for in-statement contraction is wrong because it breaks the original 
program semantic.

Am I missing something?


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added inline comments.



Comment at: clang/include/clang/Basic/LangOptions.h:396-401
+allow_reassoc(LangOpts.FastMath || LangOpts.AllowFPReassoc),
+no_nans(LangOpts.FastMath || LangOpts.NoHonorNaNs),
+no_infs(LangOpts.FastMath || LangOpts.NoHonorInfs),
+no_signed_zeros(LangOpts.FastMath || LangOpts.NoSignedZero),
+allow_reciprocal(LangOpts.FastMath || LangOpts.AllowRecip),
+approx_func(LangOpts.FastMath || LangOpts.ApproxFunc) {}

Same comment on `LangOpts.FastMath || ` as the one for `CompilerInvocation.cpp`.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3190-3196
+  Opts.AllowFPReassoc = Opts.FastMath || CGOpts.Reassociate;
+  Opts.NoHonorNaNs =
+  Opts.FastMath || CGOpts.NoNaNsFPMath || Opts.FiniteMathOnly;
+  Opts.NoHonorInfs =
+  Opts.FastMath || CGOpts.NoInfsFPMath || Opts.FiniteMathOnly;
+  Opts.NoSignedZero = Opts.FastMath || CGOpts.NoSignedZeros;
+  Opts.AllowRecip = Opts.FastMath || CGOpts.ReciprocalMath;

Why do we need `Opts.FastMath || ` here? The code in the compiler driver 
`clang/lib/Driver/ToolChains/Clang.cpp` 
(https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/Clang.cpp#L2510)
 already takes care of generating the right flags for the CC1 to configure the 
floating point rules.

Moreover, if we ignore what the compiler driver does, the fact that 
`Args.hasArg(OPT_ffast_math)` is not considered in the definition of the 
codegen options such as `NoInfsFPMath`, `NoNaNsFPMath`, `NoSignedZeros`, 
`Reassociate`, so the you have already two distinct options for the same 
abstract property that might not match.

I think that at the CC1 level the reasoning should be done in terms of the fine 
grain options, and let the compiler driver makes life easy for the users -- 
i.e. `LangOpts.FastMath` should just control whether the macro `__FAST_MATH__` 
is defined or not.



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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

IIUC, the way within-statement contraction is supposed to work is that there 
are supposed to be blocking intrinsics inserted at various places.  I don't 
remember the details, though, or if anyone's thought about how it interacts 
with cross-statement contraction, which this pragma permits within the same 
function (and could occur with inlining regardless).


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Michele Scandale via Phabricator via cfe-commits]

michele.scandale added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:227
+  FMF.setAllowContract(FPFeatures.allowFPContractAcrossStatement() ||
+   FPFeatures.allowFPContractWithinStatement());
 }

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.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I think we might have had to change that test on our fork when we changed the 
parsing order.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

@rjmccall @mibintc I think the diagnostics-order.c test is still behaving 
correctly technically. The note lines are still printing with the associated 
error lines, it just happens that one of the warning lines prints at the end 
instead of in the middle. ie:

  error: invalid value '-foo' in '-verify='
  note: -verify prefixes must start with a letter and contain only alphanumeric 
characters, hyphens, and underscores
  error: invalid value 'bogus' in '-std=bogus'
  note: use 'c89', 'c90', or 'iso9899:1990' for 'ISO C 1990' standard
  ...
  warning: optimization level '-O999' is not supported; using '-O3' instead

instead of

  error: invalid value '-foo' in '-verify='
  note: -verify prefixes must start with a letter and contain only alphanumeric 
characters, hyphens, and underscores
  warning: optimization level '-O999' is not supported; using '-O3' instead
  error: invalid value 'bogus' in '-std=bogus'
  note: use 'c89', 'c90', or 'iso9899:1990' for 'ISO C 1990' standard
  ...



In D72841#2030468 , @plotfi wrote:

> That was a good fix. I am pretty sure this does mean the diagnostics-order.c 
> will fail on apple's bots. The same diagnostics lines print, but in the wrong 
> order. I haven't root caused that yet.
>
> In D72841#2030099 , @mibintc wrote:
>
> > In D72841#2029821 , @plotfi wrote:
> >
> > > @ab @rjmccall @mibintc Posted D79730  
> > > for consideration.
> > >  @mibintc can you produce a version of _this_ diff that works with D79730 
> > >  applied. Currently the following fail, 
> > > as they do on Apple Master:
> >
> >
> > @rjmccall accepted the proposed patch https://reviews.llvm.org/D79735, so I 
> > pushed that.  I also tried your patch and the 3 CodeGen tests pass but the 
> > diagnostics-order.c test fails
> >
> > > 
> > > 
> > >   Failing Tests (4):
> > > Clang :: CodeGen/finite-math.c
> > > Clang :: CodeGen/fp-floatcontrol-stack.cpp
> > > Clang :: CodeGenOpenCL/relaxed-fpmath.cl
> > > Clang :: Frontend/diagnostics-order.c
>





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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

That was a good fix. I am pretty sure this does mean the diagnostics-order.c 
will fail on apple's bots. The same diagnostics lines print, but in the wrong 
order. I haven't root caused that yet.

In D72841#2030099 , @mibintc wrote:

> In D72841#2029821 , @plotfi wrote:
>
> > @ab @rjmccall @mibintc Posted D79730  for 
> > consideration.
> >  @mibintc can you produce a version of _this_ diff that works with D79730 
> >  applied. Currently the following fail, as 
> > they do on Apple Master:
>
>
> @rjmccall accepted the proposed patch https://reviews.llvm.org/D79735, so I 
> pushed that.  I also tried your patch and the 3 CodeGen tests pass but the 
> diagnostics-order.c test fails
>
> > 
> > 
> >   Failing Tests (4):
> > Clang :: CodeGen/finite-math.c
> > Clang :: CodeGen/fp-floatcontrol-stack.cpp
> > Clang :: CodeGenOpenCL/relaxed-fpmath.cl
> > Clang :: Frontend/diagnostics-order.c





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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D72841#2029821 , @plotfi wrote:

> @ab @rjmccall @mibintc Posted D79730  for 
> consideration.
>  @mibintc can you produce a version of _this_ diff that works with D79730 
>  applied. Currently the following fail, as 
> they do on Apple Master:


@rjmccall accepted the proposed patch https://reviews.llvm.org/D79735, so I 
pushed that.  I also tried your patch and the 3 CodeGen tests pass but the 
diagnostics-order.c test fails

> 
> 
>   Failing Tests (4):
> Clang :: CodeGen/finite-math.c
> Clang :: CodeGen/fp-floatcontrol-stack.cpp
> Clang :: CodeGenOpenCL/relaxed-fpmath.cl
> Clang :: Frontend/diagnostics-order.c




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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a subscriber: ab.
plotfi added a comment.

@ab @rjmccall @mibintc Posted D79730  for 
consideration.
@mibintc can you produce a version of _this_ diff that works with D79730 
 applied. Currently the following fail, as 
they do on Apple Master:

  Failing Tests (4):
Clang :: CodeGen/finite-math.c
Clang :: CodeGen/fp-floatcontrol-stack.cpp
Clang :: CodeGenOpenCL/relaxed-fpmath.cl
Clang :: Frontend/diagnostics-order.c



In D72841#2029309 , @mibintc wrote:

> @rjmccall Uncertain how to proceed, can you recommend?  If I recall 
> correctly, I added the lines in CompilerOptions because there were many 
> failing lit tests, i could have fixed the lit fails by adding the lang 
> options to the lit tests. (of course that change could have other 
> repercussions)





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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3192
+  Opts.NoHonorNaNs =
+  Opts.FastMath || CGOpts.NoNaNsFPMath || Opts.FiniteMathOnly;
+  Opts.NoHonorInfs =

mibintc wrote:
> @rjmccall I could set these by using Args.hasArg instead of CGOpts, would 
> that be acceptabel? 
I think so, yes.  Ideally the CG options would then be set based on the earlier 
values, or replaced with uses of the language options structure, but not having 
a direct dependency at all may be simpler.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3185
   Opts.FiniteMathOnly = Args.hasArg(OPT_ffinite_math_only) ||
   Args.hasArg(OPT_cl_finite_math_only) ||
   Args.hasArg(OPT_cl_fast_relaxed_math);

mibintc wrote:
> @rjmccall @plotfi These earlier patches are also deriving the value of 
> LangOpts from the settings of CG opts
I don't know what you mean here; the code you're quoting just seems to be 
looking at `Args`.  It's fine to re-parse arguments in both places if that 
makes something easier.  The problem is that you're looking at the 
CodeGenOptions structure itself.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3192
+  Opts.NoHonorNaNs =
+  Opts.FastMath || CGOpts.NoNaNsFPMath || Opts.FiniteMathOnly;
+  Opts.NoHonorInfs =

@rjmccall I could set these by using Args.hasArg instead of CGOpts, would that 
be acceptabel? 


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

@rjmccall Uncertain how to proceed, can you recommend?  If I recall correctly, 
I added the lines in CompilerOptions because there were many failing lit tests, 
i could have fixed the lit fails by adding the lang options to the lit tests. 
(of course that change could have other repercussions)


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 2 inline comments as done.
mibintc added a comment.

Some inline replies/comments to @rjmccall and @plotfi




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3185
   Opts.FiniteMathOnly = Args.hasArg(OPT_ffinite_math_only) ||
   Args.hasArg(OPT_cl_finite_math_only) ||
   Args.hasArg(OPT_cl_fast_relaxed_math);

@rjmccall @plotfi These earlier patches are also deriving the value of LangOpts 
from the settings of CG opts



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3187
   Args.hasArg(OPT_cl_fast_relaxed_math);
   Opts.UnsafeFPMath = Args.hasArg(OPT_menable_unsafe_fp_math) ||
   Args.hasArg(OPT_cl_unsafe_math_optimizations) ||

@rjmccall @plotfi here the codegen args are evaluated first.  Perhaps we could 
have a "reconcile codegen and lang args" function which would resolve the 
floating point settings into a final setting? so that codegen or lang could be 
parsed in either order? 


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

I will work @rjmccall comment about codegen vs langopt - can you leave it in 
place for now?


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

@rjmccall @mibintc Can we revert this patch for now then, and re-land when this 
patch is reworked? It would be good to get those bots passing. @rjmccall are 
the bots that you see failing on your end public?

In D72841#2028834 , @rjmccall wrote:

> In D72841#2027740 , @plotfi wrote:
>
> > Hi @rjmccall, I am also seeing similar failures. It is failing on apple's 
> > master (and the swift branches as well) because ParseLangArgs and 
> > ParseCodeGenArgs are getting called in the opposite order in apple/master 
> > from the order they are called in llvm/master. I posted a PR to fix those 
> > failures here: https://github.com/apple/llvm-project/pull/1202
> >
> > but I don't know if this is the most correct approach.
>
>
> Oh, thank you for figuring that out.  Yeah, it's reasonable for code-gen 
> option parsing to depend on language-option parsing, which means that this 
> patch is wrong.  The right fix is that we need to stop parsing these as 
> code-gen options, which is reasonable since they have direct 
> language-semantics impact.  If we still need the code-gen option flags, we 
> should be able to recreate them from the language options, I think.





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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D72841#2027740 , @plotfi wrote:

> Hi @rjmccall, I am also seeing similar failures. It is failing on apple's 
> master (and the swift branches as well) because ParseLangArgs and 
> ParseCodeGenArgs are getting called in the opposite order in apple/master 
> from the order they are called in llvm/master. I posted a PR to fix those 
> failures here: https://github.com/apple/llvm-project/pull/1202
>
> but I don't know if this is the most correct approach.


Oh, thank you for figuring that out.  Yeah, it's reasonable for code-gen option 
parsing to depend on language-option parsing, which means that this patch is 
wrong.  The right fix is that we need to stop parsing these as code-gen 
options, which is reasonable since they have direct language-semantics impact.  
If we still need the code-gen option flags, we should be able to recreate them 
from the language options, I think.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

Hi @rjmccall, I am also seeing similar failures. It is failing on apple's 
master (and the swift branches as well) because ParseLangArgs and 
ParseCodeGenArgs are getting called in the opposite order in apple/master from 
the order they are called in llvm/master. I posted a PR to fix those failures 
here: https://github.com/apple/llvm-project/pull/1202

but I don't know if this is the most correct approach.

In D72841#2023058 , @rjmccall wrote:

> We're also seeing test failures in Apple's Clang fork, e.g.
>
>   test/CodeGen/finite-math.c:12:10: error: NSZ: expected string not found in 
> input
>// NSZ: fadd nsz
>^
>   :11:20: note: scanning from here
>   define void @foo() #0 {
>  ^
>   :15:9: note: possible intended match here
>%add = fadd float %0, %1
>   ^
>
>
> I haven't checked yet whether we can reproduce upstream.





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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

I got a report that this patch was causing a problem with Windows  
header because #pragma float_control should be supported in namespace context.  
I've posted a patch for review here https://reviews.llvm.org/D79631


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

I posted a patch to fix the bug reported by @uabelho here 
https://reviews.llvm.org/D79510

@rjmccall I used check-clang and check-all on D71841 
 from my linux x86-64 server before 
submitting, and the testing was clear. Maybe your branch has some calls to 
create BinaryOperator that isn't passing FPFeatures with the correct 
value--related to D76384 .  My sandbox is 
showing this

  %add = fadd nsz float %0, %1


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We're also seeing test failures in Apple's Clang fork, e.g.

  test/CodeGen/finite-math.c:12:10: error: NSZ: expected string not found in 
input
   // NSZ: fadd nsz
   ^
  :11:20: note: scanning from here
  define void @foo() #0 {
 ^
  :15:9: note: possible intended match here
   %add = fadd float %0, %1
  ^

I haven't checked yet whether we can reproduce upstream.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D72841#2022354 , @uabelho wrote:

> It seems to be this change in SemaStmt.cpp that makes the FENV-pragma have 
> some effect regardless of the warning saying that the pragma  doesn't have 
> any effect:
>
>   index aa0d89ac09c3..f76994a6dab3 100644
>   @@ -394,6 +394,11 @@ StmtResult Sema::ActOnCompoundStmt(SourceLocation L, 
> SourceLocation R,
>   ArrayRef Elts, bool isStmtExpr) 
> {
>  const unsigned NumElts = Elts.size();
>   
>   +  // Mark the current function as usng floating point constrained 
> intrinsics
>   +  if (getCurFPFeatures().isFPConstrained())
>   +if (FunctionDecl *F = dyn_cast(CurContext))
>   +  F->setUsesFPIntrin(true);
>   +
>


Thank you I will follow up!


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

It seems to be this change in SemaStmt.cpp that makes the FENV-pragma have some 
effect regardless of the warning saying that the pragma  doesn't have any 
effect:

  index aa0d89ac09c3..f76994a6dab3 100644
  @@ -394,6 +394,11 @@ StmtResult Sema::ActOnCompoundStmt(SourceLocation L, 
SourceLocation R,
  ArrayRef Elts, bool isStmtExpr) {
 const unsigned NumElts = Elts.size();
   
  +  // Mark the current function as usng floating point constrained intrinsics
  +  if (getCurFPFeatures().isFPConstrained())
  +if (FunctionDecl *F = dyn_cast(CurContext))
  +  F->setUsesFPIntrin(true);
  +


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

Hi @mibintc ,

I think I'm seeing some oddities with this patch (current trunk, 0054c46095 
).
With

  clang -S -Xclang -emit-llvm bbi-42553.c -o -

on the input

  float rintf(float x);
  #pragma STDC FENV_ACCESS ON
  
  void t()
  {
  (void)rintf(0.0f);
  }

I get

  bbi-42553.c:2:14: warning: pragma STDC FENV_ACCESS ON is not supported, 
ignoring pragma [-Wunknown-pragmas]
  #pragma STDC FENV_ACCESS ON
   ^
  ; ModuleID = 'bbi-42553.c'
  source_filename = "bbi-42553.c"
  target datalayout = 
"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
  target triple = "x86_64-unknown-linux-gnu"
  
  ; Function Attrs: noinline nounwind optnone strictfp uwtable
  define dso_local void @t() #0 {
  entry:
%0 = call float @llvm.experimental.constrained.rint.f32(float 0.00e+00, 
metadata !"round.tonearest", metadata !"fpexcept.ignore") #2
ret void
  }

and if I remove the pragma I instead get

  define dso_local void @t() #0 {
  entry:
%0 = call float @llvm.rint.f32(float 0.00e+00)
ret void
  }

Before this patch I got the call to llvm.rint.f32 in both cases.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

Looks OK to me.  Please give @sepavloff and @rjmccall a day or so to make final 
comments before committing.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:870
+def err_pragma_fenv_requires_precise : Error<
+  "'#pragma STDC FENV_ACCESS ON' is illegal when precise is disabled">;
 def warn_cxx_ms_struct :

mibintc wrote:
> erichkeane wrote:
> > The last 4 can be done via selects as well!  Save a couple more spaces 
> > before we have to up the diagnostic id size :) 
> > 
> > The last 4 can be done via selects as well
> Combining these 4 into 1 diagnostic is doable but it's ugly.
Concur, I spent some time on it and don't really like it.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked an inline comment as done.
mibintc added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:870
+def err_pragma_fenv_requires_precise : Error<
+  "'#pragma STDC FENV_ACCESS ON' is illegal when precise is disabled">;
 def warn_cxx_ms_struct :

erichkeane wrote:
> The last 4 can be done via selects as well!  Save a couple more spaces before 
> we have to up the diagnostic id size :) 
> 
> The last 4 can be done via selects as well
Combining these 4 into 1 diagnostic is doable but it's ugly.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

2 small things, @rjmccall  and @sepavloff , anything else?




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:870
+def err_pragma_fenv_requires_precise : Error<
+  "'#pragma STDC FENV_ACCESS ON' is illegal when precise is disabled">;
 def warn_cxx_ms_struct :

The last 4 can be done via selects as well!  Save a couple more spaces before 
we have to up the diagnostic id size :) 




Comment at: clang/include/clang/Sema/Sema.h:558
 
+  // This stacks the current state of Sema.CurFPFeatures
+  PragmaStack FpPragmaStack;

erichkeane wrote:
> This comment is really oddly phrased and uses the 'stack'-noun as a verb?  
> Something like: (please feel free to wordsmith): "This stack tracks the 
> current state of Sema.CurFPFeatures."?
Just needs a period at the end.



Comment at: clang/lib/Parse/ParsePragma.cpp:2534
+  PP.Lex(Tok);
+  if (Tok.isNot(tok::l_paren)) {
+PP.Diag(FloatControlLoc, diag::err_expected) << tok::l_paren;

mibintc wrote:
> erichkeane wrote:
> > Replace this with BalancedDelimiterTracker instead, it gives consistent 
> > errors and are a bit easier to use.  Additionally, I think it does some 
> > fixups that allow us to recover better.
> > 
> > I'd also suggest some refactoring with the PragmaFloatControlKind if/elses 
> > below.  Perhaps handle the closing paren at the end, and do a switch-case 
> > for that handling.
> BalancedDelimiterTracker doesn't work here because there's no access to the 
> Parser object. Rewriting it would be an extensive change and I'm doubtful 
> about making this change. PragmaHandler is defined in Lex. I think there are 
> 60 pragma's that use the PragmaHandler. 
Thats unfortunate :/  That type does some nice fixup work.


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked an inline comment as done.
mibintc added inline comments.



Comment at: clang/include/clang/AST/Expr.h:2701
 
+  FPOptions FPFeatures;
+

mibintc wrote:
> erichkeane wrote:
> > This type already has trailing-storage type stuff.  I think in the past 
> > review @rjmccall encouraged you to put this in the fake-trailing-storage  
> > like above.
> whoops i meant that to go to the CallExprBits
Adding FPOptions to CallExprBits makes the field too large, I'm going to drop 
adding FPOptions to CallExpr, i'll add it via trailingstorage in a separate 
patch if it's needed.



Comment at: clang/lib/Parse/ParsePragma.cpp:2534
+  PP.Lex(Tok);
+  if (Tok.isNot(tok::l_paren)) {
+PP.Diag(FloatControlLoc, diag::err_expected) << tok::l_paren;

erichkeane wrote:
> Replace this with BalancedDelimiterTracker instead, it gives consistent 
> errors and are a bit easier to use.  Additionally, I think it does some 
> fixups that allow us to recover better.
> 
> I'd also suggest some refactoring with the PragmaFloatControlKind if/elses 
> below.  Perhaps handle the closing paren at the end, and do a switch-case for 
> that handling.
BalancedDelimiterTracker doesn't work here because there's no access to the 
Parser object. Rewriting it would be an extensive change and I'm doubtful about 
making this change. PragmaHandler is defined in Lex. I think there are 60 
pragma's that use the PragmaHandler. 



Comment at: clang/lib/Sema/SemaAttr.cpp:1023
+  Diag(Loc, diag::err_pragma_fenv_requires_precise);
 CurFPFeatures.setAllowFEnvAccess();
 break;

erichkeane wrote:
> Should we still be setting this even if there was an error?
> Should we still be setting this even if there was an error?
It's not harmful to set it, if there's an error diagnostic then there is no 
codegen right?





Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:684
 void ASTStmtReader::VisitUnaryOperator(UnaryOperator *E) {
+  bool hasFP_Features;
   VisitExpr(E);

mibintc wrote:
> erichkeane wrote:
> > Rather than this variable, why not just ask 'E' below?
> yes i could do that. it would be a function call
i made some changes in this area, eliminating setHasStoredFPFeatures


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 5 inline comments as done.
mibintc added a comment.

A couple replies to @erichkeane




Comment at: clang/include/clang/AST/Expr.h:2251
+  /// allocated in Trailing Storage
+  void setHasStoredFPFeatures(bool B) { UnaryOperatorBits.HasFPFeatures = B; }
+  bool hasStoredFPFeatures() const { return UnaryOperatorBits.HasFPFeatures; }

erichkeane wrote:
> Is this really useful/usable at all?  It seems to me that since this would 
> require re-allocating this object that noone should be able to set this after 
> construction.
It's only used during serialization (ASTReader); I guess the node has already 
been allocated by then so it's superfluous, because the allocation point could 
set this field. 



Comment at: clang/include/clang/AST/Expr.h:2268
+  FPOptions getFPFeatures(const LangOptions ) const {
+if (UnaryOperatorBits.HasFPFeatures)
+  return getStoredFPFeatures();

erichkeane wrote:
> Is there use in having both this AND the get-stored, as opposed to just 
> making everyone access via the same function?  At least having 2 public 
> versions aren't very clear what the difference is to me.
John suggested the name getStored hasStored as "less tempting" names. The 
getStored and hasStored are only used during Serialization.  John suggested the 
getFPFeatures function as the public interface and it uses the LangOptions 
parameter.  The features are saved in the node if they can't be recreated from 
the command line floating point options (due to presence of floating point 
pragma)



Comment at: clang/include/clang/AST/Expr.h:2701
 
+  FPOptions FPFeatures;
+

erichkeane wrote:
> This type already has trailing-storage type stuff.  I think in the past 
> review @rjmccall encouraged you to put this in the fake-trailing-storage  
> like above.
whoops i meant that to go to the CallExprBits



Comment at: clang/include/clang/Basic/LangOptions.h:197
   static constexpr unsigned FPR_ToNearest =
-  static_cast(llvm::RoundingMode::NearestTiesToEven);
+  static_cast(RoundingMode::NearestTiesToEven);
 

erichkeane wrote:
> Is this an unrelated change?  What is the purpose for this?
it's a NFC the llvm:: prefix wasn't needed. maybe the clang formatter did that? 



Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:684
 void ASTStmtReader::VisitUnaryOperator(UnaryOperator *E) {
+  bool hasFP_Features;
   VisitExpr(E);

erichkeane wrote:
> Rather than this variable, why not just ask 'E' below?
yes i could do that. it would be a function call


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

@rjmccall  : can you comment on the CallExpr's trailing storage issue?  And 
give advice as to what you meant in the last review?




Comment at: clang/include/clang/AST/Expr.h:2122
+return *getTrailingObjects();
+  }
+  const FPOptions () const {

needs a newline between functions.



Comment at: clang/include/clang/AST/Expr.h:2251
+  /// allocated in Trailing Storage
+  void setHasStoredFPFeatures(bool B) { UnaryOperatorBits.HasFPFeatures = B; }
+  bool hasStoredFPFeatures() const { return UnaryOperatorBits.HasFPFeatures; }

Is this really useful/usable at all?  It seems to me that since this would 
require re-allocating this object that noone should be able to set this after 
construction.



Comment at: clang/include/clang/AST/Expr.h:2255
+  /// Get FPFeatures from trailing storage
+  FPOptions getStoredFPFeatures() const {
+assert(hasStoredFPFeatures());

Why is this a separate function from getTrailingFPFeatures?  



Comment at: clang/include/clang/AST/Expr.h:2256
+  FPOptions getStoredFPFeatures() const {
+assert(hasStoredFPFeatures());
+return getTrailingFPFeatures();

If they have to be separate, the assert here doesn't really make sense, since 
getTrailingFPFeatures has the same assert.



Comment at: clang/include/clang/AST/Expr.h:2261
+  void setStoredFPFeatures(FPOptions F) {
+assert(UnaryOperatorBits.HasFPFeatures);
+getTrailingFPFeatures() = F;

For the asserts, we should probably prefer using the hasStoredFPFeatures 
function.



Comment at: clang/include/clang/AST/Expr.h:2268
+  FPOptions getFPFeatures(const LangOptions ) const {
+if (UnaryOperatorBits.HasFPFeatures)
+  return getStoredFPFeatures();

Is there use in having both this AND the get-stored, as opposed to just making 
everyone access via the same function?  At least having 2 public versions 
aren't very clear what the difference is to me.



Comment at: clang/include/clang/AST/Expr.h:2701
 
+  FPOptions FPFeatures;
+

This type already has trailing-storage type stuff.  I think in the past review 
@rjmccall encouraged you to put this in the fake-trailing-storage  like above.



Comment at: clang/include/clang/Basic/LangOptions.h:197
   static constexpr unsigned FPR_ToNearest =
-  static_cast(llvm::RoundingMode::NearestTiesToEven);
+  static_cast(RoundingMode::NearestTiesToEven);
 

Is this an unrelated change?  What is the purpose for this?



Comment at: clang/include/clang/Basic/LangOptions.h:389
   // Used for serializing.
   explicit FPOptions(unsigned I)
+  : fp_contract(I & 3), fenv_access((I >> 2) & 1), rounding((I >> 3) & 7),

Is this the same logic as getFromOpaqueInt?  If so, we should probably just 
call that.



Comment at: clang/include/clang/Sema/Sema.h:558
 
+  // This stacks the current state of Sema.CurFPFeatures
+  PragmaStack FpPragmaStack;

This comment is really oddly phrased and uses the 'stack'-noun as a verb?  
Something like: (please feel free to wordsmith): "This stack tracks the current 
state of Sema.CurFPFeatures."?



Comment at: clang/lib/AST/Expr.cpp:1309
   unsigned NumPreArgs = PreArgs.size();
+  FPFeatures = FP_Features;
   CallExprBits.NumPreArgs = NumPreArgs;

Is there a reason this isn't a member initializer?



Comment at: clang/lib/Parse/ParsePragma.cpp:667
+  uintptr_t Value = reinterpret_cast(Tok.getAnnotationValue());
+  Sema::PragmaMsStackAction Action =
+  static_cast((Value >> 16) & 0x);

Oh boy these are some magic lookin numbers... Can you document these two lines?



Comment at: clang/lib/Parse/ParsePragma.cpp:2534
+  PP.Lex(Tok);
+  if (Tok.isNot(tok::l_paren)) {
+PP.Diag(FloatControlLoc, diag::err_expected) << tok::l_paren;

Replace this with BalancedDelimiterTracker instead, it gives consistent errors 
and are a bit easier to use.  Additionally, I think it does some fixups that 
allow us to recover better.

I'd also suggest some refactoring with the PragmaFloatControlKind if/elses 
below.  Perhaps handle the closing paren at the end, and do a switch-case for 
that handling.



Comment at: clang/lib/Sema/SemaAttr.cpp:428
+  case PFC_NoExcept:
+switch (Value) {
+default:

I guess I don't get why you're switching on both here?  Can the two just be 
combined?  I don't know if the 'NewValue = CurFPFeatures.getAsOpaqueInt();
FpPragmaStack.Act(Loc, Action, StringRef(), NewValue);'

part is sufficiently motivated to do 2 separate switches.



Comment at: clang/lib/Sema/SemaAttr.cpp:1023
+  Diag(Loc, 

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked an inline comment as done.
mibintc added inline comments.



Comment at: clang/docs/LanguageExtensions.rst:3197
+by the pragma behaves as though the command-line option
+ ``-ffp-exception-behavior=strict`` is enabled,
+when ``pragma float_control(precise, off)`` is enabled, the section of code

there's an extra whitespace here, i'll get rid of it


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

noted bug in an inline comment. i will upload a fix


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

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 2 inline comments as done.
mibintc added a comment.

added a couple inline explanatory comments




Comment at: clang/include/clang/Basic/LangOptions.h:307
 
+  bool denormalIsIEEE = false;
+

I added this boolean as part of validating the correctness of the pragma's that 
access the FP environment, according to the Microsoft checks.. Copying from the 
Microsoft doc:  "There are restrictions on the ways you can use the fenv_access 
pragma in combination with other floating-point settings:

You can't enable fenv_access unless precise semantics are enabled. Precise 
semantics can be enabled either by the float_control pragma, or by using the 
/fp:precise or /fp:strict compiler options. The compiler defaults to 
/fp:precise if no other floating-point command-line option is specified.

You can't use float_control to disable precise semantics when fenv_access(on) 
is set."
This is copied from 
https://docs.microsoft.com/en-us/cpp/preprocessor/fenv-access?view=vs-2019



Comment at: clang/test/CodeGen/constrained-math-builtins.c:157
 
-// CHECK: call float @llvm.experimental.constrained.fmuladd.f32
-// CHECK: fneg
-// CHECK: call double @llvm.experimental.constrained.fmuladd.f64
-// CHECK: fneg
-// CHECK: call x86_fp80 @llvm.experimental.constrained.fmuladd.f80
+  // CHECK: call contract float @llvm.experimental.constrained.fmuladd.f32
+  // CHECK: fneg

Since this patch constructs the FPFeatures using the floating point settings 
from the command line versus the default FPOptions() constructor, several tests 
need to be changed. Some of the changes I made showing the flags on the IR, 
other tests I changed by adding ffp-contract to the RUN line to match the 
expected IR.


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