compiler cleanliness patches and incorporation of HLS APT #1470

tomeichlersmith · 2024-09-20T13:58:03Z

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

For some reason, this only was shown on trunk after merging in unrelated (but in TrigScint?) code #1461 .

Check List

I successfully compiled ldmx-sw with my developments
I ran my developments and the following shows that they are successful.

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

tomeichlersmith · 2024-09-20T13:59:10Z

Canceled the PR Validation since I am just making sure this builds with warnings-as-errors.

tvami · 2024-09-20T14:00:23Z

For some reason, this only was shown on trunk

Is it because the branch Rory used was not rebased to the version of trunk that had my update for the CI?

tomeichlersmith · 2024-09-20T14:01:38Z

That's why I think this wasn't caught while test-building that branch, but I still don't know why this wasn't caught in previous rounds of more strict building since this code has been there for awhile. 🤷

Doesn't really matter, this is an easy fix.

tomeichlersmith · 2024-09-20T14:15:55Z

crap, this just revealed more issues

https://github.com/LDMX-Software/ldmx-sw/actions/runs/10960598567/job/30435575518?pr=1470

the good news is that it builds and runs when not treating warnings-as-errors and so its not super urgent

…umbering of digi collections

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

tvami · 2024-09-20T14:40:55Z

Some of the new stuff is in HLS_arbitrary_Precision_Types, this line https://github.com/LDMX-Software/ldmx-sw/blob/trunk/CMakeLists.txt#L37C36-L37C65 was supressing those in the past but I think with this addition

ldmx-sw/TrigScint/CMakeLists.txt

Lines 32 to 37 in 5598eaa

    
           setup_library(module TrigScint name Event 
        
                         dependencies ROOT::Core 
        
                                      Hcal::Event 
        
           	     TrigScint::Event 
        
                                      Recon::Event 
        
                         register_target)

it's resurfucing again

tomeichlersmith · 2024-09-20T14:48:45Z

I fixed up our internal stuff, all that's left is the HLS stuff.

/home/tom/code/ldmx/ldmx-sw/TrigScint/../Trigger/HLS_arbitrary_Precision_Types/include/etc/ap_private.h:2109:32: warning: The left operand of '<<' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
            ? ((((int64_t)VAL) << (excess_bits)) >> (excess_bits))
                               ^
/home/tom/code/ldmx/ldmx-sw/TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx:293:14: note: Calling 'operator+<12, true, 12, true>'
  float pe = outTrk.Pad1.Seed.Amp + outTrk.Pad1.Sec.Amp;
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/tom/code/ldmx/ldmx-sw/TrigScint/../Trigger/HLS_arbitrary_Precision_Types/include/ap_int_base.h:1320:1: note: Calling constructor for 'ap_int_base<13, true>'
OP_BIN_AP(+, plus)
^
/home/tom/code/ldmx/ldmx-sw/TrigScint/../Trigger/HLS_arbitrary_Precision_Types/include/ap_int_base.h:1310:37: note: expanded from macro 'OP_BIN_AP'
        _AP_W2, _AP_S2>::Rty##_base lhs(op);                                  \
                                    ^~~~~~~
/home/tom/code/ldmx/ldmx-sw/TrigScint/../Trigger/HLS_arbitrary_Precision_Types/include/ap_int_base.h:179:10: note: Calling default constructor for 'ssdm_int_sim<13, true>'
  INLINE ap_int_base(const ap_int_base<_AP_W2, _AP_S2>& op) {
         ^~~~~~~~~~~
/home/tom/code/ldmx/ldmx-sw/TrigScint/../Trigger/HLS_arbitrary_Precision_Types/include/ap_common.h:248:3: note: Calling default constructor for 'ap_private<13, true, true>'
  ssdm_int_sim() {}
  ^~~~~~~~~~~~
/home/tom/code/ldmx/ldmx-sw/TrigScint/../Trigger/HLS_arbitrary_Precision_Types/include/etc/ap_private.h:1595:5: note: Calling 'ap_private::clearUnusedBits'
    clearUnusedBits();
    ^~~~~~~~~~~~~~~~~
/home/tom/code/ldmx/ldmx-sw/TrigScint/../Trigger/HLS_arbitrary_Precision_Types/include/etc/ap_private.h:2108:9: note: '?' condition is true
        _AP_S
        ^
/home/tom/code/ldmx/ldmx-sw/TrigScint/../Trigger/HLS_arbitrary_Precision_Types/include/etc/ap_private.h:2109:32: note: The left operand of '<<' is a garbage value
            ? ((((int64_t)VAL) << (excess_bits)) >> (excess_bits))
                          ~~~  ^

which, as you point out @tvami , can probably be silenced by informing CMake that the HLS headers are not our problem.

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

TrigScint/include/TrigScint/TrigScintFirmwareTracker.h

tvami · 2024-09-24T21:15:10Z

So in the end, this is not just something we can hide under the rug, there is some issue in the input in

TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx

the track level seems fine
but either at the outTrk.Pad1 or outTrk.Pad1.Seed or outTrk.Pad1.Seed.Amp (same with the Sec) there is something uninitialized. I didnt find it tho :(

tomeichlersmith · 2024-09-24T21:38:01Z

This gave me the idea to just use {} within the struct declarations telling C++ to use value initialization which avoids nonsense (random memory) values. e.g.

ldmx-sw/TrigScint/include/TrigScint/Firmware/objdef.h

Lines 26 to 30 in 6c08a49

    
           struct Digi { 
        
             int mID, bID; 
        
             int adc0, adc1, adc2, adc3, adc4, adc5; 
        
             int tdc0, tdc1, tdc2, tdc3, tdc4, tdc5; 
        
           };

I /think/ this got past the error on my local computer so I'm pushing it here to double check.

Edit: nevermind. This didn't work locally.

tvami · 2024-09-24T21:50:08Z

I was thinking if we could just set the defaults it would fix it, but given that this is C-style, I dont think we can do that. I think having

ap_int<12> mID{0}, bID{-1};

would actually solve the problem, but we cant do that, right?

tomeichlersmith · 2024-09-24T21:54:43Z

I don't know if this code needs to be C-style or if that was just most convenient when trying to write the emulation.

tvami · 2024-09-24T22:00:34Z

OK, I tried, it doesnt work anyway... :(

tvami · 2024-09-24T22:55:45Z

I tried if getting rid of calcTCent would resolve it, but then I get this (meaning that the issue roots in constructing the lookup table )

/home/vamitamas/patch-float-for-loop-counter/ldmx-sw/TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx:63:3: note: Loop condition is true.  Entering loop body
  for (int i = 0; i < NCENT; i++) {
  ^
/home/vamitamas/patch-float-for-loop-counter/ldmx-sw/TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx:64:5: note: Loop condition is true.  Entering loop body
    for (int j = 0; j < COMBO; j++) {
    ^
/home/vamitamas/patch-float-for-loop-counter/ldmx-sw/TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx:65:26: note: Calling 'operator-<12, true>'
      LOOKUP[i][j][0] = (i - A[1] + A[0]);
                         ^~~~~~~~

tvami · 2024-09-24T22:59:08Z

And honestly I dont understand what's happening here

  ap_int<12> A[3] = {0, 0, 0};
  ap_int<12> LOOKUP[NCENT][COMBO][2];

  LOOKUP[i][j][0] = (i - A[1] + A[0]);

isnt A[1] the same as A[0] and both are zeros?

rodwyer100 · 2024-09-24T23:23:11Z

A[i] is an alignment matrix. The idea is that the three layers (Pad1, Pad2, and Pad3) may be translated w.r.t eachother and said translation may be determined by the vector A as we only care how they are misaligned in one axis (the axis of granularity of the TS). The factor (i - A[1] + A[0]) only becomes nontrivial if A[1] and A[0] are different indicating an initially misaligned state. This will be important when we first start checking LDMX because if there is alignment issues we wont see it without scanning A.

tvami · 2024-09-24T23:48:11Z

I see, thanks @rodwyer100 !
Would it be ok to initialize the LOOKUP table to all 0 before you fill in the real values?

tvami · 2024-09-25T00:21:58Z

ehh

/home/vamitamas/patch-float-for-loop-counter/ldmx-sw/TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx:62:3: note: Loop condition is true.  Entering loop body
  for (int i = 0; i < NCENT; ++i) {
  ^
    for (int j = 0; j < COMBO; ++j) {
    ^
      for (int k = 0; k < 2; ++k) {
      ^
/home/vamitamas/patch-float-for-loop-counter/ldmx-sw/TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx:65:31: note: Calling constructor for 'ap_int<12>'
            LOOKUP[i][j][k] = ap_int<12>(0);
                              ^~~~~~~~~~~~~

it doesnt like anything with ap_int<12> constructor.

tvami · 2024-09-25T01:03:25Z

hey @tomeichlersmith @rodwyer100 @bryngemark
At this point I think we should bring the HLS submodule to be part of the ldmx-sw, and fix the VAL thing (I did it locally and it resolves it). The HLS repo was last touched 5 years ago, and it's not very big [funny thing that Shazhad from CMS did try to patch it a year ago -- the PR is still unmerged].
I also think it would have a natural place in Framework or under Tools, given that it's used both by Trigger and TrigScint

rodwyer100 · 2024-09-25T01:04:01Z

It should be okay to initialize it with certain values. I didn't understand your most current comment. Did that not work?

rodwyer100 · 2024-09-25T01:04:14Z

Sorry now your second to most current. Edit: Ah I see. It doesn't like the ap_int<12> instantiation. I don't see why it wouldn't; Ill look at it but it should be allowed.

…ults

tvami

If pushed in some changes that I explored to fix the problem. Although they did not fix it, I think it still makes sense to have them in, and this PR could be a general fixup PR. I tagged Rory for some specific questions below.

Otherwise I think we should merge this, have a separate PR for the inclusion of the HLS stuff

TrigScint/include/TrigScint/Firmware/objdef.h

TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx

tvami · 2024-09-25T03:49:28Z

TrigScint/src/TrigScint/TrigScintFirmwareTracker.cxx

+  for (int i = 0; i < NCENT; ++i) {
+    for (int j = 0; j < COMBO; ++j) {
+      for (int k = 0; k < 2; ++k) {
+        LOOKUP[i][j][k] = ap_int<12>(0);


I hope putting this to zero is fine

I will clone this branch and check gimme a sec. The logic in the array does mean some default values will be bad, but 0 may be an okay one.

tvami · 2024-09-25T03:50:10Z

TrigScint/include/TrigScint/Firmware/objdef.h

  float one = (float)c.Pad1.Cent;
  float two = (float)c.Pad2.Cent;
  float three = (float)c.Pad3.Cent;
  float mean = (one + two + three) / 3.0;
-  ap_int<12> Cent = (ap_int<10>)((int)(mean));
+  ap_int<12> Cent = (ap_int<12>)((int)(mean));


I really think this should be as I change it, but please check

This will not significantly affect the firmware part of this equation. If it agrees still on track positions, it will also work. I will clone this branch and check gimme a sec.

rodwyer100 · 2024-09-25T06:40:23Z

So something has been scrambled. I don't know which change did it, but I will need to inspect things before this gets commited. The track number distribution is very high; it is producing a high fake rate.

rodwyer100 · 2024-09-25T06:47:43Z

I will need to look event by event to see why this occurs.

tomeichlersmith · 2024-09-25T14:18:04Z

Personally, I would rather keep a fork of HLS_arbitrary_precision and just redirect our submodule to our fork.

Similar to G4DarkBreM, I just think there is a possibility that this code would be used in other applications (LDMX or not) and so keeping it separate would be helpful.

tvami · 2024-09-25T14:51:28Z

rather keep a fork of HLS_arbitrary_precision

but it's such a small repo... having it as a submodule has the advantage that if it changes we can just update it whenever we think it's needed. But this did not and does not change even when it should (see the PR from Shahzad a year ago). And then for your argument about using in other applications: for outside ldmx I doubt people would take an ldmx-sw fork instead of having their own fork. And then for inside ldmx, isnt that an argument to have it in our software directly?

tomeichlersmith · 2024-09-25T15:04:29Z

Yea.. you've convinced me

I'll put them in Tools and update the CMake

…aders

Xilinx/HLS_arbitrary_Precision_Types#1

Tools/HLS_Arbitrary_Precision_Types/include/ap_impl/ap_private.h

tvami

Nice! We are now back to be able to compile with the strict requirements. I hope Rory can find what change changed the track distributions

tomeichlersmith · 2024-09-25T20:11:05Z

@rodwyer100 I am going to merge this even though it certainly contains the issues that you have observed just because there is a lot of code being introduced. I think your other PR #1473 can be a location for patches while you are introducing the hit stagger.

rodwyer100 · 2024-09-26T03:45:59Z

I figured out what happened. Its a small change thats needed which won't change what was done. I will include it in my hit branch alongside a rebased branch

dont use a float as a for-loop counter

05ff7e4

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

tomeichlersmith added 2 commits September 20, 2024 09:36

remove unnecessary member variables holding digis, rename and align n…

2e584fe

…umbering of digi collections

more compiler warning patches

b4b4929

- 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

0ad2f87

github-actions bot and others added 2 commits September 20, 2024 14:49

Apply clang-format

840f7c3

dont specify HLS includes as a specific directory

05b052f

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

tvami reviewed Sep 20, 2024

View reviewed changes

TrigScint/include/TrigScint/TrigScintFirmwareTracker.h Show resolved Hide resolved

tomeichlersmith mentioned this pull request Sep 23, 2024

Committing TS Firmware Hit Reconstruction Stagger for the Purpose of Triggering Studies #1473

Merged

2 tasks

tvami mentioned this pull request Sep 24, 2024

Update OS to Ubuntu 24.04 and ROOT to 6.32.04 LDMX-Software/docker#103

Open

4 tasks

require value initialization in Firmware-mimic structs

8a387d5

Add checks in TrigScintFirmwareTracker, and fix array sizes, add defa…

237d26c

…ults

Resolve conflict

a850b8a

tvami reviewed Sep 25, 2024

View reviewed changes

tomeichlersmith and others added 12 commits September 25, 2024 11:02

copy HLS APT submodule into ldmx-sw proper under Tools

986e826

update readme notes

194d07c

rename etc and hide private headers in the ap_impl directory

f296659

fix up paths in includes so examples compile

e9d54ff

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

6092212

remove inclusion of directory that doesn't exist anymore

c7094fe

patch inclusion of directories to correctly specify them as system he…

5af4b1e

…aders

cleanup TS CMakeLists and specify Tools as dependency of Firmware

ac82a91

add patch from PR #1

a19fa5d

Xilinx/HLS_arbitrary_Precision_Types#1

wrap unknown pragmas in ifdef so they can be ignored here

c59f45b

remove unused centroid variable

c28ee81

Apply clang-format

bb7f5e3

tvami reviewed Sep 25, 2024

View reviewed changes

Tools/HLS_Arbitrary_Precision_Types/include/ap_impl/ap_private.h Outdated Show resolved Hide resolved

tomeichlersmith changed the title ~~dont use a float as a for-loop counter~~ compiler cleanliness patches and incorporation of HSL APT Sep 25, 2024

tomeichlersmith changed the title ~~compiler cleanliness patches and incorporation of HSL APT~~ compiler cleanliness patches and incorporation of HLS APT Sep 25, 2024

tomeichlersmith marked this pull request as ready for review September 25, 2024 19:26

tvami approved these changes Sep 25, 2024

View reviewed changes

tomeichlersmith merged commit ebbf27a into trunk Sep 25, 2024
11 of 16 checks passed

tomeichlersmith deleted the patch-float-for-loop-counter branch September 25, 2024 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler cleanliness patches and incorporation of HLS APT #1470

compiler cleanliness patches and incorporation of HLS APT #1470

tomeichlersmith commented Sep 20, 2024

tomeichlersmith commented Sep 20, 2024

tvami commented Sep 20, 2024

tomeichlersmith commented Sep 20, 2024

tomeichlersmith commented Sep 20, 2024

tvami commented Sep 20, 2024

tomeichlersmith commented Sep 20, 2024

tvami commented Sep 24, 2024

tomeichlersmith commented Sep 24, 2024 •

edited

Loading

tvami commented Sep 24, 2024 •

edited

Loading

tomeichlersmith commented Sep 24, 2024

tvami commented Sep 24, 2024

tvami commented Sep 24, 2024

tvami commented Sep 24, 2024

rodwyer100 commented Sep 24, 2024

tvami commented Sep 24, 2024

tvami commented Sep 25, 2024 •

edited

Loading

tvami commented Sep 25, 2024

rodwyer100 commented Sep 25, 2024

rodwyer100 commented Sep 25, 2024 •

edited

Loading

tvami left a comment

tvami Sep 25, 2024

rodwyer100 Sep 25, 2024

tvami Sep 25, 2024

rodwyer100 Sep 25, 2024

rodwyer100 commented Sep 25, 2024

rodwyer100 commented Sep 25, 2024

tomeichlersmith commented Sep 25, 2024

tvami commented Sep 25, 2024

tomeichlersmith commented Sep 25, 2024

tvami left a comment

tomeichlersmith commented Sep 25, 2024

rodwyer100 commented Sep 26, 2024

compiler cleanliness patches and incorporation of HLS APT #1470

compiler cleanliness patches and incorporation of HLS APT #1470

Conversation

tomeichlersmith commented Sep 20, 2024

Check List

tomeichlersmith commented Sep 20, 2024

tvami commented Sep 20, 2024

tomeichlersmith commented Sep 20, 2024

tomeichlersmith commented Sep 20, 2024

tvami commented Sep 20, 2024

tomeichlersmith commented Sep 20, 2024

tvami commented Sep 24, 2024

tomeichlersmith commented Sep 24, 2024 • edited Loading

tvami commented Sep 24, 2024 • edited Loading

tomeichlersmith commented Sep 24, 2024

tvami commented Sep 24, 2024

tvami commented Sep 24, 2024

tvami commented Sep 24, 2024

rodwyer100 commented Sep 24, 2024

tvami commented Sep 24, 2024

tvami commented Sep 25, 2024 • edited Loading

tvami commented Sep 25, 2024

rodwyer100 commented Sep 25, 2024

rodwyer100 commented Sep 25, 2024 • edited Loading

tvami left a comment

Choose a reason for hiding this comment

tvami Sep 25, 2024

Choose a reason for hiding this comment

rodwyer100 Sep 25, 2024

Choose a reason for hiding this comment

tvami Sep 25, 2024

Choose a reason for hiding this comment

rodwyer100 Sep 25, 2024

Choose a reason for hiding this comment

rodwyer100 commented Sep 25, 2024

rodwyer100 commented Sep 25, 2024

tomeichlersmith commented Sep 25, 2024

tvami commented Sep 25, 2024

tomeichlersmith commented Sep 25, 2024

tvami left a comment

Choose a reason for hiding this comment

tomeichlersmith commented Sep 25, 2024

rodwyer100 commented Sep 26, 2024

tomeichlersmith commented Sep 24, 2024 •

edited

Loading

tvami commented Sep 24, 2024 •

edited

Loading

tvami commented Sep 25, 2024 •

edited

Loading

rodwyer100 commented Sep 25, 2024 •

edited

Loading