Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove unnecessary gcc-specific optimization flags #4156

Merged
merged 1 commit into from
Jan 17, 2019
Merged

Remove unnecessary gcc-specific optimization flags #4156

merged 1 commit into from
Jan 17, 2019

Conversation

nbhuiyan
Copy link
Member

@nbhuiyan nbhuiyan commented Dec 31, 2018 •
edited
Loading

Using -fno-rounding-math and -fno-signalling-nans flags on OSX
results in Clang warning about these unsupported options for
every use of this option.

Signed-off-by: Nazim Uddin Bhuiyan nazimudd@ualberta.ca

Edit: Also removed these options for GCC due to them being defaults.

Edit2: Since #4118 addressed the clang warning issues, this PR has been modified to address just removing the options on GCC.

@nbhuiyan nbhuiyan mentioned this pull request Dec 31, 2018
Open
@DanHeidinga
Copy link
Member

Jenkins test sanity osx jdk8,jdk11

DanHeidinga
DanHeidinga approved these changes Jan 1, 2019
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@DanHeidinga DanHeidinga self-assigned this Jan 1, 2019
@andrewcraik
Copy link
Contributor

@DanHeidinga and @nbhuiyan - we do need to be careful that the floating point rounding mode is correct without the -f option and that NANs don't trap. These bugs can be really subtle so we need to be sure what the Clang mode is with these options off otherwise the JIT can encounter incorrect floating point behaviour which is an absolute nightmare to debug (speaking from experience). FYI @0dvictor since you added these options originally.

@0dvictor
Copy link
Contributor

0dvictor commented Jan 2, 2019 •
edited
Loading

Quote from LLVM's documentation: (http://llvm.org/docs/LangRef.html)

The default LLVM floating-point environment assumes that floating-point instructions do not have side effects. Results assume the round-to-nearest rounding mode. No floating-point exception state is maintained in this environment. Therefore, there is no attempt to create or preserve invalid operation (SNaN) or division-by-zero exceptions.

The behavior should be same as GCC's -fno-rounding-math and -fno-signaling-nans. In addition, as -fno-rounding-math and -fno-signaling-nans are default on GCC, we can safely remove them for GCC as well.

@nbhuiyan
Copy link
Member Author

nbhuiyan commented Jan 2, 2019

Thanks for looking into this, @0dvictor!

If we were to remove these options for gcc as well, I think it's necessary to understand the original motivations behind specifying -fno-signaling-nans and -fno-rounding-math options for gcc. Was it necessary to specify these options in the version of gcc used at the time they were added? In that case, we may need to verify that the versions of gcc currently used for OpenJ9 builds do have these options by default.

@0dvictor
Copy link
Contributor

0dvictor commented Jan 3, 2019 •
edited
Loading

Was it necessary to specify these options in the version of gcc used at the time they were added?

No, it wasn't necessary. The four floating point related GCC options are directly inferred from The Java Language Specification on floating point semantics, and hence were added.
IMO, when this file was only consumed when compiling with GCC, it is fine to add extra but non-essential options. Now Clang also uses this file and not support these two options, it is completely unnecessary to selectively add these two to GCC. It also introduces confusion as readers may think Clang behaves differently from GCC.

we may need to verify that the versions of gcc currently used for OpenJ9 builds do have these options by default.

Good suggestion, I've checked GCC versions from 4.4.7 to 8.2, -fno-signaling-nans and -fno-signaling-nans are all default.

@nbhuiyan nbhuiyan force-pushed the macos-clang-warnings branch from cab7090 to 127319f Compare January 3, 2019 18:13
DanHeidinga
DanHeidinga approved these changes Jan 4, 2019
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated change to completely remove the options looks good.

@DanHeidinga
Copy link
Member

Jenkins test sanity osx jdk8,jdk11

@DanHeidinga
Copy link
Member

@0dvictor Are you OK with the updated change?

@DanHeidinga
Copy link
Member

Jenkins test sanity xlinux jdk8

0dvictor
0dvictor approved these changes Jan 4, 2019
View reviewed changes
Copy link
Contributor

@0dvictor 0dvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DanHeidinga
Copy link
Member

I see #4118 was merged which addresses the same issue as this PR. The only difference being that it kept the options for non-clang compiles.

If it's important to remove the options for gcc as well, then @nbhuiyan can you rebase this PR and remove the bit of from #4118?

@nbhuiyan
Remove unnecessary gcc-specific optimization flags
f8222df 
-fno-rounding-math and -fno-signaling-nans options were made
specific to GCC due to resulting in warnings on Clang. In Clang,
these options are not supported, but it does assume
round-to-nearest mode and use non-signalling NaNs by default. In
GCC, these 2 options are set by default, and hence it is not
necessary to specify them. Removing these options entirely from
the makefile will prevent any potential confusion about the
default behavior on Clang and GCC.

Signed-off-by: Nazim Uddin Bhuiyan <nazimudd@ualberta.ca>
@nbhuiyan nbhuiyan force-pushed the macos-clang-warnings branch from 127319f to f8222df Compare January 15, 2019 15:29
@nbhuiyan nbhuiyan changed the title Disable use of unsupported optimization flags in OSX JIT builds Remove unnecessary gcc-specific optimization flags Jan 15, 2019
@nbhuiyan
Copy link
Member Author

@DanHeidinga
As pointed out by @0dvictor , selectively using these options on GCC could potentially cause confusion about the default behavior on Clang and GCC. Therefore, I have removed these options entirely from the makefile.

0dvictor
0dvictor approved these changes Jan 15, 2019
View reviewed changes
Copy link
Contributor

@0dvictor 0dvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated change LGTM.

DanHeidinga
DanHeidinga approved these changes Jan 17, 2019
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@DanHeidinga
Copy link
Member

Jenkins test sanity xlinux jdk8

@DanHeidinga DanHeidinga merged commit 8eb6f83 into eclipse-openj9:master Jan 17, 2019
babsingh added a commit to babsingh/openj9 that referenced this pull request Aug 16, 2019
@babsingh
Re-add "-fno-rounding-math" and "-fno-signaling-nans" options in JIT
c732261 
The above compiler options were removed in
eclipse-openj9#4156.

Removing the above compiler options causes a performance regression.
Symptoms of the performance regression: bursts of high throughput mixed
with periods of lower throughput. These symptoms cause a 5-7% throughput
regression.

Reintroducing the above compiler options fixes the throughput
regression.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh babsingh mentioned this pull request Aug 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanHeidinga DanHeidinga DanHeidinga approved these changes

@0dvictor 0dvictor 0dvictor approved these changes

Assignees

@DanHeidinga DanHeidinga

Labels
comp:build comp:jit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nbhuiyan @DanHeidinga @andrewcraik @0dvictor