Warning for partially implemented options/pragmas?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Warning for partially implemented options/pragmas?

Fangrui Song via cfe-dev
Bug 45329 “segfault with frounding-math” “https://bugs.llvm.org/show_bug.cgi?id=45329” highlights that we are silently allowing people to use options that are unimplemented or only partially implemented.
 
If use the -frounding-math option on a compile I expect it to work. I do not expect it to trigger an ICE, a crash, or much worse silently not work and produce wrong results.
 
I propose a warning be printed so that we can avoid issues like bug 45329 in the future. And it needs to be per-target since some targets are closer to supporting this feature than others.
 
Thoughts?
--
Kevin P. Neal
SAS/C and SAS/C++ Compiler
Compute Services
SAS Institute, Inc.
 
 
 
 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning for partially implemented options/pragmas?

Fangrui Song via cfe-dev
On Thu, Apr 16, 2020 at 4:01 PM Kevin Neal via cfe-dev
<[hidden email]> wrote:
>
> Bug 45329 “segfault with frounding-math” “https://bugs.llvm.org/show_bug.cgi?id=45329” highlights that we are silently allowing people to use options that are unimplemented or only partially implemented.
>
> If use the -frounding-math option on a compile I expect it to work. I do not expect it to trigger an ICE, a crash, or much worse silently not work and produce wrong results.
>
> I propose a warning be printed so that we can avoid issues like bug 45329 in the future. And it needs to be per-target since some targets are closer to supporting this feature than others.
>
> Thoughts?

Thanks for bringing this up, Kevin.

We have a customer code that has been using
`-fno-unsafe-math-optimizations` for some time. That option implies
`-ftrapping-math`, which was a no-op until a few months ago. Now,
`-ftrapping-math` enables the constrained FP intrinsics, which aren't
fully supported on all targets. That behavior change looked like a
regression to the customer.

A warning would be a good short-term solution.

-Cameron
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning for partially implemented options/pragmas?

Fangrui Song via cfe-dev
I think it would be quite useful to add this in the front end. The way I envision this working is that targets would specify whether they support constrained FP intrinsics. If this flag is unset, it would cause the front end to do two things:
1. Warn the user that the mode is not supported for the target
2. Ignore the option and not generate the constrained FP intrinsics

This should additionally be coupled with a temporary option for developers to use to aid in developing support for the intrinsics.
Something like: -ftrapping-math-on-unsupported-target and -frounding-math-on-unsupported-target.


On Thu, Apr 16, 2020 at 4:19 PM Cameron McInally via cfe-dev <[hidden email]> wrote:
On Thu, Apr 16, 2020 at 4:01 PM Kevin Neal via cfe-dev
<[hidden email]> wrote:
>
> Bug 45329 “segfault with frounding-math” “https://bugs.llvm.org/show_bug.cgi?id=45329” highlights that we are silently allowing people to use options that are unimplemented or only partially implemented.
>
> If use the -frounding-math option on a compile I expect it to work. I do not expect it to trigger an ICE, a crash, or much worse silently not work and produce wrong results.
>
> I propose a warning be printed so that we can avoid issues like bug 45329 in the future. And it needs to be per-target since some targets are closer to supporting this feature than others.
>
> Thoughts?

Thanks for bringing this up, Kevin.

We have a customer code that has been using
`-fno-unsafe-math-optimizations` for some time. That option implies
`-ftrapping-math`, which was a no-op until a few months ago. Now,
`-ftrapping-math` enables the constrained FP intrinsics, which aren't
fully supported on all targets. That behavior change looked like a
regression to the customer.

A warning would be a good short-term solution.

-Cameron
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning for partially implemented options/pragmas?

Fangrui Song via cfe-dev

I’m already working on support for target-specific handling.

 

I like the idea of target-specific disabling of the relevant options. I do think a warning is appropriate since this is a _correctness_ issue, and also because the options can be triggered indirectly like Cameron showed.

 

The target-specific parts in clang itself that need to be done are very small, and as far as I know are confined to CGBuiltins.cpp. So I don’t think that any new options need to be added for developer use. Testing is an issue, but X86 and SystemZ are in such good shape that clang tests for anything else besides builtins can test on one of those two targets.

 

From: Nemanja Ivanovic <[hidden email]>
Sent: Thursday, April 23, 2020 10:01 AM
To: [hidden email]
Cc: Kevin Neal <[hidden email]>; Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Warning for partially implemented options/pragmas?

 

EXTERNAL

I think it would be quite useful to add this in the front end. The way I envision this working is that targets would specify whether they support constrained FP intrinsics. If this flag is unset, it would cause the front end to do two things:

1. Warn the user that the mode is not supported for the target

2. Ignore the option and not generate the constrained FP intrinsics

 

This should additionally be coupled with a temporary option for developers to use to aid in developing support for the intrinsics.

Something like: -ftrapping-math-on-unsupported-target and -frounding-math-on-unsupported-target.

 

 

On Thu, Apr 16, 2020 at 4:19 PM Cameron McInally via cfe-dev <[hidden email]> wrote:

On Thu, Apr 16, 2020 at 4:01 PM Kevin Neal via cfe-dev
<[hidden email]> wrote:
>
> Bug 45329 “segfault with frounding-math” “https://bugs.llvm.org/show_bug.cgi?id=45329” highlights that we are silently allowing people to use options that are unimplemented or only partially implemented.
>
> If use the -frounding-math option on a compile I expect it to work. I do not expect it to trigger an ICE, a crash, or much worse silently not work and produce wrong results.
>
> I propose a warning be printed so that we can avoid issues like bug 45329 in the future. And it needs to be per-target since some targets are closer to supporting this feature than others.
>
> Thoughts?

Thanks for bringing this up, Kevin.

We have a customer code that has been using
`-fno-unsafe-math-optimizations` for some time. That option implies
`-ftrapping-math`, which was a no-op until a few months ago. Now,
`-ftrapping-math` enables the constrained FP intrinsics, which aren't
fully supported on all targets. That behavior change looked like a
regression to the customer.

A warning would be a good short-term solution.

-Cameron
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning for partially implemented options/pragmas?

Fangrui Song via cfe-dev
The reason I suggested developer options was so that back end developers can build large applications using the option to get the intrinsics emitted and then chase what falls out (without changing the front end).
For example, on PPC, I could specify the option, build the application that we know causes crashes in the back end for us and continue the work to implement full support.

But as you mentioned, the changes in the front end to flip the flag to say PPC supports this and continue my testing would be trivial.

On Thu, Apr 23, 2020 at 1:53 PM Kevin Neal <[hidden email]> wrote:

I’m already working on support for target-specific handling.

 

I like the idea of target-specific disabling of the relevant options. I do think a warning is appropriate since this is a _correctness_ issue, and also because the options can be triggered indirectly like Cameron showed.

 

The target-specific parts in clang itself that need to be done are very small, and as far as I know are confined to CGBuiltins.cpp. So I don’t think that any new options need to be added for developer use. Testing is an issue, but X86 and SystemZ are in such good shape that clang tests for anything else besides builtins can test on one of those two targets.

 

From: Nemanja Ivanovic <[hidden email]>
Sent: Thursday, April 23, 2020 10:01 AM
To: [hidden email]
Cc: Kevin Neal <[hidden email]>; Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Warning for partially implemented options/pragmas?

 

EXTERNAL

I think it would be quite useful to add this in the front end. The way I envision this working is that targets would specify whether they support constrained FP intrinsics. If this flag is unset, it would cause the front end to do two things:

1. Warn the user that the mode is not supported for the target

2. Ignore the option and not generate the constrained FP intrinsics

 

This should additionally be coupled with a temporary option for developers to use to aid in developing support for the intrinsics.

Something like: -ftrapping-math-on-unsupported-target and -frounding-math-on-unsupported-target.

 

 

On Thu, Apr 16, 2020 at 4:19 PM Cameron McInally via cfe-dev <[hidden email]> wrote:

On Thu, Apr 16, 2020 at 4:01 PM Kevin Neal via cfe-dev
<[hidden email]> wrote:
>
> Bug 45329 “segfault with frounding-math” “https://bugs.llvm.org/show_bug.cgi?id=45329” highlights that we are silently allowing people to use options that are unimplemented or only partially implemented.
>
> If use the -frounding-math option on a compile I expect it to work. I do not expect it to trigger an ICE, a crash, or much worse silently not work and produce wrong results.
>
> I propose a warning be printed so that we can avoid issues like bug 45329 in the future. And it needs to be per-target since some targets are closer to supporting this feature than others.
>
> Thoughts?

Thanks for bringing this up, Kevin.

We have a customer code that has been using
`-fno-unsafe-math-optimizations` for some time. That option implies
`-ftrapping-math`, which was a no-op until a few months ago. Now,
`-ftrapping-math` enables the constrained FP intrinsics, which aren't
fully supported on all targets. That behavior change looked like a
regression to the customer.

A warning would be a good short-term solution.

-Cameron
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Warning for partially implemented options/pragmas?

Fangrui Song via cfe-dev

Oh, I can see that. We’ve got an intern that’s been testing as well.

 

I wouldn’t oppose you adding that option. I don’t know what anyone else thinks, though.

 

Right now my problem is I know very little about clang’s driver. So that’s where I’m working at the moment.

--
Kevin P. Neal
SAS/C and SAS/C++ Compiler

Compute Services

SAS Institute, Inc.

 

 

 

From: Nemanja Ivanovic <[hidden email]>
Sent: Friday, April 24, 2020 6:38 AM
To: Kevin Neal <[hidden email]>
Cc: [hidden email]; Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Warning for partially implemented options/pragmas?

 

EXTERNAL

The reason I suggested developer options was so that back end developers can build large applications using the option to get the intrinsics emitted and then chase what falls out (without changing the front end).

For example, on PPC, I could specify the option, build the application that we know causes crashes in the back end for us and continue the work to implement full support.

 

But as you mentioned, the changes in the front end to flip the flag to say PPC supports this and continue my testing would be trivial.

 

On Thu, Apr 23, 2020 at 1:53 PM Kevin Neal <[hidden email]> wrote:

I’m already working on support for target-specific handling.

 

I like the idea of target-specific disabling of the relevant options. I do think a warning is appropriate since this is a _correctness_ issue, and also because the options can be triggered indirectly like Cameron showed.

 

The target-specific parts in clang itself that need to be done are very small, and as far as I know are confined to CGBuiltins.cpp. So I don’t think that any new options need to be added for developer use. Testing is an issue, but X86 and SystemZ are in such good shape that clang tests for anything else besides builtins can test on one of those two targets.

 

From: Nemanja Ivanovic <[hidden email]>
Sent: Thursday, April 23, 2020 10:01 AM
To: [hidden email]
Cc: Kevin Neal <[hidden email]>; Clang Dev <[hidden email]>
Subject: Re: [cfe-dev] Warning for partially implemented options/pragmas?

 

EXTERNAL

I think it would be quite useful to add this in the front end. The way I envision this working is that targets would specify whether they support constrained FP intrinsics. If this flag is unset, it would cause the front end to do two things:

1. Warn the user that the mode is not supported for the target

2. Ignore the option and not generate the constrained FP intrinsics

 

This should additionally be coupled with a temporary option for developers to use to aid in developing support for the intrinsics.

Something like: -ftrapping-math-on-unsupported-target and -frounding-math-on-unsupported-target.

 

 

On Thu, Apr 16, 2020 at 4:19 PM Cameron McInally via cfe-dev <[hidden email]> wrote:

On Thu, Apr 16, 2020 at 4:01 PM Kevin Neal via cfe-dev
<[hidden email]> wrote:
>
> Bug 45329 “segfault with frounding-math” “https://bugs.llvm.org/show_bug.cgi?id=45329” highlights that we are silently allowing people to use options that are unimplemented or only partially implemented.
>
> If use the -frounding-math option on a compile I expect it to work. I do not expect it to trigger an ICE, a crash, or much worse silently not work and produce wrong results.
>
> I propose a warning be printed so that we can avoid issues like bug 45329 in the future. And it needs to be per-target since some targets are closer to supporting this feature than others.
>
> Thoughts?

Thanks for bringing this up, Kevin.

We have a customer code that has been using
`-fno-unsafe-math-optimizations` for some time. That option implies
`-ftrapping-math`, which was a no-op until a few months ago. Now,
`-ftrapping-math` enables the constrained FP intrinsics, which aren't
fully supported on all targets. That behavior change looked like a
regression to the customer.

A warning would be a good short-term solution.

-Cameron
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev