Skip to content

Conversation

@P-Jensen
Copy link
Contributor

@P-Jensen P-Jensen commented Jul 6, 2022

Gives a small performance benefit for Gromacs on NVidia targets

@P-Jensen P-Jensen requested review from a team as code owners July 6, 2022 21:39
case options::OPT_fgpu_flush_denormals_to_zero:
DenormalFPMath = llvm::DenormalMode::getPositiveZero();
DenormalFP32Math = llvm::DenormalMode::getPositiveZero();
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a driver test verifying the expect values when the options are set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdtoguchi Do you prefer a separate test for this, or should I add it to the clang/test/Driver/fp-model.c?

@premanandrao
Copy link
Contributor

Please add a CodeGen test for this.

@bader
Copy link
Contributor

bader commented Jul 7, 2022

@andykaylor, FYI.

mdtoguchi
mdtoguchi previously approved these changes Jul 7, 2022
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

OK for driver

@againull againull requested a review from premanandrao August 12, 2022 17:56
@againull
Copy link
Contributor

@premanandrao Please review this PR.

@elizabethandrews
Copy link
Contributor

@premanandrao Please review this PR.

@premanandrao is OOO next week . I see he has reviewed the patch earlier and requested a CodeGen test which hasn't been added yet

@P-Jensen
Copy link
Contributor Author

@elizabethandrews @againull This is my bad. I'll add a test case next week.

@P-Jensen
Copy link
Contributor Author

@elizabethandrews @againull I just added a CodeGen test case.

Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

FE changes look good to me.

@mdtoguchi
Copy link
Contributor

@intel/llvm-gatekeepers, this looks ready to go.

@steffenlarsen steffenlarsen merged commit e8e7ae8 into intel:sycl Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants