Skip to content

ap_private: Initialize data member in default constructor to avoid compilation warnings #1

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smuzaffar
Copy link

This change fixes the compilation warnings like [a]. Both VAL and pVal are properly initialized in all ap_private constructor except the default one. This PR explicitly initialize these data members on default constructor too.

[a]

In member function 'clearUnusedBits',
    inlined from '__ct ' at hls//include/etc/ap_private.h:1595:20,
    inlined from '__ct_base ' at hls//include/ap_common.h:248:18,
    inlined from '__ct ' at hls//include/ap_int_base.h:208:3,
    inlined from 'operator&' at hls//include/ap_int_base.h:1436:1,
    inlined from 'clusterEnergy' at cmssw/src/L1Trigger/L1CaloTrigger/interface/Phase2L1CaloEGammaUtils.h:872:54,
    inlined from 'compareClusterET' at cmssw/src/L1Trigger/L1CaloTrigger/interface/Phase2L1CaloEGammaUtils.h:962:52:
  hls//include/etc/ap_private.h:2109:27: warning: 'MEM[(volatile struct ap_private *)&D.9255].VAL' is used uninitialized [-Wuninitialized]
  2109 |             ? ((((int64_t)VAL) << (excess_bits)) >> (excess_bits))
In member function 'clearUnusedBits',
    inlined from '__ct ' at /data/cmsbld/jenkins/workspace/build-any-ib/w/el8_amd64_gcc12/external/hls/2019.08-b8a1533230929513077d97b1507ff465/include/etc/ap_private.h:3305:20,
    inlined from '__ct_base ' at hls/include/ap_common.h:248:18,
    inlined from '__ct ' at hls/include/ap_int_base.h:179:61,
    inlined from 'operator-' at hls/include/ap_int_base.h:1321:1:
  hls/include/etc/ap_private.h:3397:33: warning: 'MEM[(volatile struct ap_private *)&lhs].pVal[1]' is used uninitialized [-Wuninitialized]
  3397 |         _AP_S ? ((((int64_t)pVal[_AP_N - 1]) << (excess_bits)) >> excess_bits)

@tvami
Copy link

tvami commented Sep 25, 2024

@luciferlee @MattSnow-amd can you please merge this? I see it's been open for a year now.

tomeichlersmith added a commit to LDMX-Software/ldmx-sw that referenced this pull request Sep 25, 2024
tomeichlersmith added a commit to LDMX-Software/ldmx-sw that referenced this pull request Sep 25, 2024
* dont use a float as a for-loop counter

replace this with a while loop instead to be more clear about the possibility of rounding errors affecting performance

* remove unnecessary member variables holding digis, rename and align numbering of digi collections

* more compiler warning patches

- update digis{1,2,3} collection variable names
- remove useless (int) casts
- remove unused int index = count variables

* remove unnecessary zero-ing of count after its use

* Apply clang-format

* dont specify HLS includes as a specific directory

we include them as a SYSTEM include in central CMakeLists.txt
and specifying them as SYSTEM informs CMake and its downstream compilers
to avoid analyzing them since they aren't our problem

* require value initialization in Firmware-mimic structs

* Add checks in TrigScintFirmwareTracker, and fix array sizes, add defaults

* copy HLS APT submodule into ldmx-sw proper under Tools

* update readme notes

* rename etc and hide private headers in the ap_impl directory

* fix up paths in includes so examples compile

* attach HLS APT headers as 'system' headers to Tools module

* remove inclusion of directory that doesn't exist anymore

* patch inclusion of directories to correctly specify them as system headers

* cleanup TS CMakeLists and specify Tools as dependency of Firmware

* add patch from PR #1

Xilinx/HLS_arbitrary_Precision_Types#1

* wrap unknown pragmas in ifdef so they can be ignored here

* remove unused centroid variable

* Apply clang-format

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Tamas Vami <[email protected]>
@fwinkl
Copy link

fwinkl commented Apr 15, 2025

We (ATLAS experiment at CERN) are also very much interested in getting this merged. And thanks to our CMS colleagues for upstreaming their fix.

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.

3 participants