Removed undefined use of union in libminifloat by Dr15Jones · Pull Request #35054 · cms-sw/cmssw

Dr15Jones · 2021-08-27T18:51:24Z

PR description:

Testing showed use of memcpy produced identical assembly code.

PR validation:

Code compiles and unit test pass.

Testing showed use of memcpy produced identical assembly code.

Dr15Jones · 2021-08-27T18:52:05Z

The test case can be tried here

https://godbolt.org/z/9qT9s4hP5

Dr15Jones · 2021-08-27T18:54:09Z

Using a union to switch between bit representation is undefined behavior in C and C++. The only allowed way to do that is through std::memcpy. Fortunately, all known compilers know about std::memcpy and will optimize the call out and replace it with the most efficient conversion possible.

When we can use C++ 20, we can switch to std::bit_set which would allow us to make these function constexpr.

cmsbuild · 2021-08-27T18:56:45Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35054/24928

This PR adds an extra 16KB to repository

cmsbuild · 2021-08-27T18:57:07Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

DataFormats/Math (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @fabiocos, @rovere this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2021-08-27T19:40:19Z

please test

cmsbuild · 2021-08-27T23:07:23Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bba71/18105/summary.html
COMMIT: 18a6fa5
CMSSW: CMSSW_12_1_X_2021-08-27-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35054/18105/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bba71/18105/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bba71/18105/git-merge-result

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000330
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-08-28T00:11:27Z

Reco comparison results: 9 differences found in the comparisons

it looks like these are from #34860, but better to rerun after the new IB shows up

slava77 · 2021-08-28T13:05:39Z

@cmsbuild please test

cmsbuild · 2021-08-28T17:07:38Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bba71/18114/summary.html
COMMIT: 18a6fa5
CMSSW: CMSSW_12_1_X_2021-08-27-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35054/18114/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000324
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-08-28T21:08:43Z

DataFormats/Math/interface/libminifloat.h

  }

 private:
+  //in C++20 we can use std::bit_cast which is constexpr


is it worthwhile to fold this in #ifndef __cpp_lib_bit_cast and #include <bit> at the top if it's defined?

I took a look at https://en.cppreference.com/w/cpp/feature_test and it seems to me like the feature test system is really meant for libraries which are meant to support many different C++ versions simultaneously. That isn't what we really do. Once we validate C++20 option for our compilers (which we haven't even started since no compiler supports all the items we might want) we will not need to compile that code with an older compiler.

So personally, I don't see a compelling reason to add 10+ lines of code (as a guess) which are only useful during the short window between the C++17 to C++20 transition. If you really want that ability, I will change the code.

@slava77 are you 'OK' with my reasoning about not using the test feature or would you prefer them to be added to the code?

I have a more broad question: what are we going to do with all other use cases of union?
e.g. DataFormats/Math/interface/FastMath.h, SIMDVec.h, approx_math.h

the bit decoding is also used quite extensively in DataFormats/GEMDigi, e.g. interface/GEMVFAT.h; is that part OK, or does it need to switch to memcpy as well?

@slava77 I'm of the opinion that we should change all the uses of union for bit decoding eventually. It would be nice to have the bit_cast for that but I'd be OK with doing the memcpy.

The only place we discovered memcpy to be a problem is when used with the NVidia compiler as that one still calls the memcpy function rather than replacing it with inlined machine code. So for GPU shared code, we have to wait for the bit_cast support (in gcc/clang and ncc) before we can remove the undefined behavior.

I was thinking that just calling memcpy in the other cases directly was how I was going to handle it. I first tried it that way here, but there was so much repetition that I switched to defining a bit_cast (which drastically simplified the code, surprisingly).

If we were to have our own bit_cast it would more likely be put in FWCore/Utilities since we've often put soon to come to std items there in the past.

As for the other places in DataFormat/Math I'm not sure the compilers are smart enough to be able to vectorize the replacements for memcpy they use. So I'd skip those for now.

If we were to have our own bit_cast it would more likely be put in FWCore/Utilities since we've often put soon to come to std items there in the past.

I'm certainly fine with FWCore/Utilities

Do you want us to make the bit_cast separately for this pull request or can we wait until we see a need?

Do you want us to make the bit_cast separately for this pull request or can we wait until we see a need?

from #35054 (comment)
I concluded that there is a need (there just isn't another PR yet that addresses other use cases of a union).

cmsbuild · 2021-09-01T21:17:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35054/25010

This PR adds an extra 20KB to repository

cmsbuild · 2021-09-01T21:18:20Z

Pull request #35054 was updated. @smuzaffar, @Dr15Jones, @makortel, @cmsbuild, @slava77, @jpata can you please check and sign again.

slava77 · 2021-09-01T22:47:51Z

@cmsbuild please test

cmsbuild · 2021-09-02T02:09:24Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8bba71/18236/summary.html
COMMIT: 1df0f46
CMSSW: CMSSW_12_1_X_2021-09-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35054/18236/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

136.72413136.72413_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMININANO_data2016UL_HIPM_met+HARVESTDR2_REMININANO_data2016UL_HIPM_met/step2_RunJetHT2016B_reminiaodUL+RunJetHT2016B_reminiaodUL+REMININANO_data2016UL_HIPM_met+HARVESTDR2_REMININANO_data2016UL_HIPM_met.log

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000404
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3000376
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

slava77 · 2021-09-02T12:57:40Z

+reconstruction

for #35054 1df0f46

code changes in line with the PR description and the follow up review
jenkins tests pass and comparisons with the baseline show no differences (apparently the actual undefined behavior did not happen)
- the failure in 136.72413 is not related to this PR

makortel · 2021-09-02T13:09:35Z

+core

cmsbuild · 2021-09-02T13:09:57Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

qliphy · 2021-09-02T13:43:48Z

+1

qliphy · 2021-09-02T13:45:13Z

merge

Removed undefined use of union in libminifloat

18a6fa5

Testing showed use of memcpy produced identical assembly code.

cmsbuild added this to the CMSSW_12_1_X milestone Aug 27, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Aug 27, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 27, 2021

cmsbuild added tests-started and removed tests-pending labels Aug 27, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 27, 2021

cmsbuild added tests-started and removed tests-approved labels Aug 28, 2021

cmsbuild added tests-approved and removed tests-started labels Aug 28, 2021

slava77 reviewed Aug 28, 2021

View reviewed changes

cmsbuild added code-checks-pending and removed tests-approved code-checks-approved labels Sep 1, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 1, 2021

cmsbuild added tests-started and removed tests-pending labels Sep 1, 2021

cmsbuild added tests-rejected and removed tests-started labels Sep 2, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 2, 2021

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Sep 2, 2021

cmsbuild added orp-approved and removed orp-pending labels Sep 2, 2021

cmsbuild merged commit c9da596 into cms-sw:master Sep 2, 2021

makortel mentioned this pull request Sep 2, 2021

Remove undefined use of union for type punning cms-sw/framework-team#255

Closed

2 tasks

This was referenced Sep 2, 2021

Added esConsumes to modules in SimMuon/MCTruth #35134

Merged

[GCC11] Fix compilation warning for CommonTools #35144

Merged

[GCC11] Fix compilation warning for EcalMatacqDigi #35148

Merged

[GCC11] Fix compilation warning for FW/ToyIntProducers #35143

Merged

Dr15Jones deleted the nounion_libminifloat branch September 9, 2021 17:48

Conversation

Dr15Jones commented Aug 27, 2021

PR description:

PR validation:

Uh oh!

Dr15Jones commented Aug 27, 2021

Uh oh!

Dr15Jones commented Aug 27, 2021

Uh oh!

cmsbuild commented Aug 27, 2021

Uh oh!

cmsbuild commented Aug 27, 2021

Uh oh!

Dr15Jones commented Aug 27, 2021

Uh oh!

cmsbuild commented Aug 27, 2021

Comparison Summary

Uh oh!

slava77 commented Aug 28, 2021

Uh oh!

slava77 commented Aug 28, 2021

Uh oh!

cmsbuild commented Aug 28, 2021

Comparison Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmsbuild commented Sep 1, 2021

Uh oh!

cmsbuild commented Sep 1, 2021

Uh oh!

slava77 commented Sep 1, 2021

Uh oh!

cmsbuild commented Sep 2, 2021

RelVals-INPUT

Comparison Summary

Uh oh!

slava77 commented Sep 2, 2021

Uh oh!

makortel commented Sep 2, 2021

Uh oh!

cmsbuild commented Sep 2, 2021

Uh oh!

qliphy commented Sep 2, 2021

Uh oh!

qliphy commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants