Fix radix sort test #34929

fwyzard · 2021-08-18T07:37:41Z

PR description:

Fix the compilation errors in HeterogeneousCore/CUDAUtilities/test/radixSort_t.cu:

take into account the actual size of the type being sorted, instead of always assuming a 64-bit type;
use an union instead of a cast to keep the compiler happy about the aliasing rules.

PR validation:

gpuRadixSort_t compiles and runs fine.

Take into account the actual size of the type being sorted. Use an union instead of a cast to keep the compiler happy about the aliasing rules.

fwyzard · 2021-08-18T07:37:58Z

type bugfix

fwyzard · 2021-08-18T07:38:25Z

please test

fwyzard · 2021-08-18T07:39:05Z

please test with cms-externals/eigen-git-mirror#7 for CMSSW_12_1_X/slc7_amd64_gcc11

fwyzard · 2021-08-18T07:41:09Z

+heterogeneous

cmsbuild · 2021-08-18T07:43:05Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34929/24737

This PR adds an extra 12KB to repository

cmsbuild · 2021-08-18T07:43:25Z

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

HeterogeneousCore/CUDAUtilities (heterogeneous)

can you please review it and eventually sign? Thanks.
@makortel, @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

fwyzard · 2021-08-18T07:46:05Z

Fixes #34917 .

cmsbuild · 2021-08-18T11:13:00Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d04a7e/17851/summary.html
COMMIT: 0b6eae2
CMSSW: CMSSW_12_1_X_2021-08-17-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34929/17851/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-d04a7e/17851/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d04a7e/17851/git-merge-result

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 12 differences found in the comparisons
DQMHistoTests: Total files compared: 39
DQMHistoTests: Total histograms compared: 3000352
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3000318
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 38 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 165 log files, 37 edm output root files, 39 DQM output files
TriggerResults: no differences found

perrotta · 2021-08-18T11:48:38Z

+1

fwyzard · 2021-08-18T11:56:54Z

The slc7_amd64_gcc11 test (where this was actually broken) is still running...

perrotta · 2021-08-18T12:23:08Z

The slc7_amd64_gcc11 test (where this was actually broken) is still running...

Ah, sorry Andrea. I saw the message that the tests ended succesfully (together with your "+1" for the heterogeneous category) and I thought they were the ones.
Ok, let see how does the relevant test end up, and stay ready to revert in case of issues

fwyzard · 2021-08-18T12:37:34Z

No problem, testing locally it worked, so I'm optimistic :-)

cmsbuild · 2021-08-18T14:57:14Z

-1

Failed Tests: Build HeaderConsistency
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d04a7e/17853/summary.html
COMMIT: 0b6eae2
CMSSW: CMSSW_12_1_X_2021-08-16-1100/slc7_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34929/17853/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-d04a7e/17853/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d04a7e/17853/git-merge-result

Build

I found compilation error when building:

>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/src/DTTrig.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/src/DTTrigProd.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/src/DTTrigTest.cc
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/src/SealModule.cc
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/src/DTTrigTest.cc: In member function 'virtual void DTTrigTest::beginRun(const edm::Run&, const edm::EventSetup&)':
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/src/DTTrigTest.cc:198:23: error: 'this' pointer is null [-Werror=nonnull]
  198 |     my_trig->createTUs(iEventSetup);
      |     ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/interface/DTTrigTest.h:26,
                 from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/src/DTTrigTest.cc:17:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_1_X_2021-08-16-1100/src/L1Trigger/DTTrigger/interface/DTTrig.h:78:8: note: in a call to non-static member function 'void DTTrig::createTUs(const edm::EventSetup&)'

fwyzard · 2021-08-18T15:05:13Z

OK, there are other (preexisting, I assume) errors in CondCore/EcalPlugins, FastSimulation/TrackingRecHitProducer and L1Trigger/DTTrigger - but indeed HeterogeneousCore/CUDAUtilities looks good.

Dr15Jones · 2021-08-18T20:05:18Z

HeterogeneousCore/CUDAUtilities/test/radixSort_t.cu

+    uintT_t<T> u;
+  } c;
+  c.t = t;
+  c.u = c.u >> shift << shift;


I'm pretty sure this is actually undefined behavior as you are not supposed to set a union using one type and read the union using another type. I believe the recommend way to do this is to use memcpy.

uintT_t<T> u; memcpy(&u, &t, sizeof(u)); u = u >>shift <<shift; memcpy(&t, &u, sizeof(t));

I played around with that recently on godbolt (for reasons other than this PR) and found compilers know about memcpy and optimize out the calls and just do the 'right' thing.

I think section 11.5.6.3 of the C++ 20 standard demonstrates that it is undefined behavior.

I'm pretty sure the union pattern is used elsewhere in CMSSW too, so we should address those as well.

fwyzard · 2021-08-18T22:17:49Z

IMHO the C++ standard has kicked itself in a corner with the strict aliasing rules, and is now clutching at straws. Personally, I'm fine with undefined behaviour, as long as the compiler does what I want it to do without complaining. If you want to propose an implementation based on memcpy and memset, go ahead - it may even simplify the code avoiding the shifts (but please add an assert on the endianness of the machine). .A

Dr15Jones · 2021-08-18T22:42:52Z

Looks like C++ 20 is finally trying to address this

https://en.cppreference.com/w/cpp/numeric/bit_cast

fwyzard · 2021-08-19T08:13:12Z

I've done some tests using memcpy instead a union, or using memset:

template <int N, typename T, typename SFINAE = std::enable_if_t<N <= sizeof(T)>>
void truncate1(T& t) {
    const int shift = 8 * (sizeof(T) - N);
    union {
        T t;
        uintT_t<T> u;
    } c;
    c.t = t;
    c.u = c.u >> shift << shift;
    t = c.t;
}

template <int N, typename T, typename SFINAE = std::enable_if_t<N <= sizeof(T)>>
void truncate2(T& t) {
    const int shift = 8 * (sizeof(T) - N);
    uintT_t<T> u;
    std::memcpy(&u, &t, sizeof(T));
    u = u >> shift << shift;
    std::memcpy(&t, &u, sizeof(T));
}

template <int N, typename T, typename SFINAE = std::enable_if_t<N <= sizeof(T)>>
void truncate3(T& t) {
    char* bytes = reinterpret_cast<char*>(&t);
    static_assert(__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__, "Only little endian architectures are supported");
    std::memset(bytes, 0x00, sizeof(T) - N);
}

Host compilers (GCC 11.2, clang 12.0) always generate the same code for truncate1 and truncate2:

GCC always generates an and with a mask
clang generates either an and with a mask, or some combinations of move and shifts, or directly sets a byte/word/dword to 0

Device compilers (NVCC 11.3, clang 11.0 --cuda) show more variety:

both generate an and with a mask for truncate1
NVCC does not optimise out the calls to memcpy in truncate2, resulting in very verbose and inefficient code
clang always generate the same code as for truncate1 (unlike on the host)

All compilers (host and device) always generate one or more stores of 0 for truncate3.

fwyzard · 2021-08-19T08:16:24Z

Summary

in host code we could replace the union and assignment with memcpy - the result does not change.
in device code we should avoid the memcpy
in all cases, it's not clear to me if memset is better or worse than the union

So... keep things as they are for the time being, and revisit once we have C++20 ?

fwyzard · 2021-08-19T08:17:17Z

P.S. my play area on godbolt is here: https://godbolt.org/z/75csPcK1q

makortel · 2021-08-19T15:25:56Z

So... keep things as they are for the time being, and revisit once we have C++20 ?

I'd be fine with that. Hopefully we get into C++20 before we need to consider other compilers than gcc or clang for pieces of code using union for type punning (that appears to be guaranteed behavior in gcc, I didn't find quickly what clang guarantees).

Fix radix sort test

0b6eae2

Take into account the actual size of the type being sorted. Use an union instead of a cast to keep the compiler happy about the aliasing rules.

cmsbuild added this to the CMSSW_12_1_X milestone Aug 18, 2021

cmsbuild added bug-fix code-checks-pending heterogeneous-pending orp-pending pending-signatures tests-pending labels Aug 18, 2021

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

fwyzard mentioned this pull request Aug 18, 2021

Fix radix sort test (12.0.x) #34930

Merged

cmsbuild added fully-signed heterogeneous-approved and removed pending-signatures heterogeneous-pending labels Aug 18, 2021

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

This was referenced Aug 18, 2021

More Eigen GPU work, with fix for CUDA 11.4 cms-externals/eigen-git-mirror#7

Merged

[GCC11] HeterogeneousCore/CUDAUtilities: array subscript is partly outside array bounds #34917

Closed

cmsbuild added the requires-external label Aug 18, 2021

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

cmsbuild added orp-approved and removed orp-pending labels Aug 18, 2021

cmsbuild merged commit 05c2b42 into cms-sw:master Aug 18, 2021

cmsbuild mentioned this pull request Aug 18, 2021

Update pileupCal.py using numpy #34931

Merged

fwyzard mentioned this pull request Aug 18, 2021

[all] Fix radix sort test cms-patatrack/pixeltrack-standalone#209

Merged

Dr15Jones reviewed Aug 18, 2021

View reviewed changes

fwyzard deleted the fix_radix_sort_test_121x branch August 19, 2021 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix radix sort test #34929

Fix radix sort test #34929

fwyzard commented Aug 18, 2021

fwyzard commented Aug 18, 2021

fwyzard commented Aug 18, 2021

fwyzard commented Aug 18, 2021 •

edited

Loading

fwyzard commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

fwyzard commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

perrotta commented Aug 18, 2021

fwyzard commented Aug 18, 2021 •

edited

Loading

perrotta commented Aug 18, 2021

fwyzard commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

fwyzard commented Aug 18, 2021

Dr15Jones Aug 18, 2021

Dr15Jones Aug 18, 2021

makortel Aug 18, 2021

fwyzard commented Aug 18, 2021 via email

Dr15Jones commented Aug 18, 2021

fwyzard commented Aug 19, 2021

fwyzard commented Aug 19, 2021

fwyzard commented Aug 19, 2021

makortel commented Aug 19, 2021

Fix radix sort test #34929

Fix radix sort test #34929

Conversation

fwyzard commented Aug 18, 2021

PR description:

PR validation:

fwyzard commented Aug 18, 2021

fwyzard commented Aug 18, 2021

fwyzard commented Aug 18, 2021 • edited Loading

fwyzard commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

fwyzard commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

Comparison Summary

perrotta commented Aug 18, 2021

fwyzard commented Aug 18, 2021 • edited Loading

perrotta commented Aug 18, 2021

fwyzard commented Aug 18, 2021

cmsbuild commented Aug 18, 2021

Build

fwyzard commented Aug 18, 2021

Dr15Jones Aug 18, 2021

Choose a reason for hiding this comment

Dr15Jones Aug 18, 2021

Choose a reason for hiding this comment

makortel Aug 18, 2021

Choose a reason for hiding this comment

fwyzard commented Aug 18, 2021 via email

Dr15Jones commented Aug 18, 2021

fwyzard commented Aug 19, 2021

fwyzard commented Aug 19, 2021

fwyzard commented Aug 19, 2021

makortel commented Aug 19, 2021

fwyzard commented Aug 18, 2021 •

edited

Loading

fwyzard commented Aug 18, 2021 •

edited

Loading