cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard

C++20 and deprecation some uses of `volatile`

Open iarspider opened this issue 1 year ago • 11 comments

In C++20, many uses of volatile keyword are deprecated, see Proposal 1152. This creates warnings in C++20 IBs:

src/DataFormats/Math/test/FastMath_t.cpp: In instantiation of 'void {anonymous}::sampleSquare() [with T = float]':
src/DataFormats/Math/test/FastMath_t.cpp:108:22:   required from here
  src/DataFormats/Math/test/FastMath_t.cpp:71:23: warning: compound assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile]
    71 |                 dummy += yy;  // add a bit of random instruction
      |                 ~~~~~~^~~~~
src/DataFormats/Math/test/FastMath_t.cpp: In instantiation of 'void {anonymous}::sampleSquare() [with T = double]':
src/DataFormats/Math/test/FastMath_t.cpp:109:23:   required from here
  src/DataFormats/Math/test/FastMath_t.cpp:71:23: warning: compound assignment with 'volatile'-qualified left operand is deprecated [-Wvolatile]
<...>
In file included from src/DataFormats/Math/test/testAtan2.cpp:9:
  src/DataFormats/Math/test/rdtscp.h:31:10: warning: 'volatile'-qualified return type is deprecated [-Wvolatile]
    31 |   inline volatile unsigned long long rdtsc() { return __rdtscp(&rdtscp_val); }
      |          ^~~~~~~~
<...>
In file included from src/DataFormats/Math/test/testAtan2.cpp:9:
  src/DataFormats/Math/test/rdtscp.h:31:10: warning: 'volatile'-qualified return type is deprecated [-Wvolatile]
    31 |   inline volatile unsigned long long rdtsc() { return __rdtscp(&rdtscp_val); }
      |          ^~~~~~~~
<...>
In file included from src/DataFormats/Math/test/testAtan2.cpp:9:
  src/DataFormats/Math/test/rdtscp.h:31:10: warning: 'volatile'-qualified return type is deprecated [-Wvolatile]
    31 |   inline volatile unsigned long long rdtsc() { return __rdtscp(&rdtscp_val); }
      |          ^~~~~~~~
<...>
In file included from src/DataFormats/Math/test/testApproxMath.cpp:137:
  src/DataFormats/Math/test/rdtscp.h:31:10: warning: 'volatile'-qualified return type is deprecated [-Wvolatile]
    31 |   inline volatile unsigned long long rdtsc() { return __rdtscp(&rdtscp_val); }
      |          ^~~~~~~~
src/CondCore/Utilities/bin/conddb_test_gt_perf.cpp: In member function 'void FetchWorker::runFake()':
  src/CondCore/Utilities/bin/conddb_test_gt_perf.cpp:360:20: warning: '++' expression of 'volatile'-qualified type is deprecated [-Wvolatile]
   360 |   void runFake() { fooGlobal++; }
      |                    ^~~~~~~~~
src/CondCore/Utilities/bin/conddb_test_gt_perf.cpp: In member function 'void DeserialWorker::runFake()':
  src/CondCore/Utilities/bin/conddb_test_gt_perf.cpp:406:20: warning: '++' expression of 'volatile'-qualified type is deprecated [-Wvolatile]
   406 |   void runFake() { fooGlobal++; }
      |                    ^~~~~~~~~

iarspider avatar May 28 '24 12:05 iarspider

assign DataFormats/Math, CondCore/Utilities

iarspider avatar May 28 '24 12:05 iarspider

New categories assigned: reconstruction,db

@jfernan2,@mandrenguyen,@francescobrivio,@saumyaphor4252,@perrotta,@consuegs you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild avatar May 28 '24 12:05 cmsbuild

cms-bot internal usage

cmsbuild avatar May 28 '24 12:05 cmsbuild

A new Issue was created by @iarspider.

@antoniovilela, @Dr15Jones, @smuzaffar, @makortel, @rappoccio, @sextonkennedy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

cmsbuild avatar May 28 '24 12:05 cmsbuild

Now that we capture CUDA warnings, there are some more uses of volatile reported: https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/el8_amd64_gcc12/CMSSW_14_1_CPP20_X_2024-06-14-1100/HeterogeneousCore/CUDAUtilities, https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/el8_amd64_gcc12/CMSSW_14_1_CPP20_X_2024-06-14-1100/CUDADataFormats/TrackingRecHit . @cms-sw/heterogeneous-l2 could you please check?

iarspider avatar Jun 18 '24 09:06 iarspider

According to the CUDA documentation:

The compiler is free to optimize reads and writes to global or shared memory (for example, by caching global reads into registers or L1 cache) as long as it respects the memory ordering semantics of memory fence functions (Memory Fence Functions) and memory visibility semantics of synchronization functions (Synchronization Functions).

These optimizations can be disabled using the volatile keyword: If a variable located in global or shared memory is declared as volatile, the compiler assumes that its value can be changed or used at any time by another thread and therefore any reference to this variable compiles to an actual memory read or write instruction.

So, if volatile was needed in the CUDA code, the options are

  • rewrite the code from
    co[i] += ws[warpId - 1];  // line 78
    ...
    co[i] += psum[k];         // line 183
    
    to
    co[i] = co[i] + ws[warpId - 1];  // line 78
    ...
    co[i] = co[i] + psum[k];         // line 183
    
    which honestly is just stupid
  • or disable the warning (hint hint)

Interestingly, the alpaka version does not use volatile. So now I'm concerned that it may be incorrect :-/

fwyzard avatar Jun 18 '24 09:06 fwyzard

or disable the warning (hint hint)

volatile compound-assignment statements are planned to be removed in C++26: P2866R0

iarspider avatar Jun 18 '24 10:06 iarspider

So we will care about it in 2030.

fwyzard avatar Jun 18 '24 10:06 fwyzard

volatile compound-assignment statements are planned to be removed in C++26: P2866R0

Actually, are you sure ?

Following the C++20 deprecations, the C committee looked to adopt a similar stance on volatile and were given feedback that a number of vendors were strongly opposed to the deprecation of compound-assignment operators, as among other reasons, many hardware APIs and device drivers would expect to use volatile compound assignment to communicate with their devices. This subset of the deprecated functionality was undeprecated for C++23 by [P2327R1], followed by further undeprecations in [CWG2654]

CWG2654 says that compound-assignment statements have been un-deprecated in C++ 23.

fwyzard avatar Jun 18 '24 10:06 fwyzard

Contrary to the direction desired in the NB comment, EWG resolved to un-deprecate all volatile compound assignments

I've read that they only decided to un-deprecate bitwise compound operators. Then yes, we can silence this warning.

iarspider avatar Jun 18 '24 10:06 iarspider

so as suggested by the compiler

Remark: The warnings can be suppressed with "-diag-suppress <warning-number>"

we can add -diag-suppress 3012 in https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_14_1_X/master/scram-tools.file/tools/cuda/cuda.xml#L14 to suppress this

smuzaffar avatar Jun 18 '24 11:06 smuzaffar

I see that 14_2_X does not have yet the -diag-suppress Is there anybody against it?

jfernan2 avatar Oct 30 '24 16:10 jfernan2

No objections, but wasn't https://github.com/cms-sw/cmsdist/pull/9252 already merged in 14.1.x ?

fwyzard avatar Oct 30 '24 16:10 fwyzard