Skip to content
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.

Sign up for GitHub

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

Attempt to fix Future L1 Track SW + bugs in HLS memory classes. #347

Merged
merged 19 commits into from
Dec 4, 2024
Merged

Attempt to fix Future L1 Track SW + bugs in HLS memory classes. #347

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2d73c03
Fix compile errors for templated functions
tomalin Sep 13, 2024
67b785f
Fixed clashes with Future SW code
tomalin Sep 17, 2024
4e8034c
fix to earlier commit
tomalin Sep 18, 2024
f2af504
Modify write_mem so it can handle old txt files that dont have page n…
tomalin Sep 18, 2024
e04b77b
Fix bug that only one quarter of memory being checked
tomalin Sep 24, 2024
54ec7db
Fix writing outside array bug
tomalin Sep 26, 2024
680159c
tweak
tomalin Sep 28, 2024
c4c8ac3
Optimised CPU use
tomalin Oct 1, 2024
d229b00
fix compilation issue
tomalin Oct 1, 2024
34d1e99
tweak
tomalin Oct 2, 2024
705052c
Fix bug in this PR
tomalin Oct 2, 2024
a3255b7
Fixed bug in PR
tomalin Oct 2, 2024
9f4b729
Fixed bug in PR
tomalin Oct 2, 2024
9d95849
Fix bug in PR
tomalin Oct 2, 2024
4beb80b
Switch to MemPrintSplit input files for Future SW
tomalin Oct 9, 2024
a4b91c1
Changed TP tp used LutsSplit with CombinedCM chain
tomalin Oct 14, 2024
d8ed714
Give variable better name
tomalin Nov 15, 2024
bdac786
Give variable better name
tomalin Nov 15, 2024
83b5bec
Replace tabs by spaces
tomalin Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions TestBenches/FileReadUtility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,9 @@ unsigned int compareMemWithMemPage(const MemType& memory_ref, const MemType& mem
constexpr int lsb = (LSB >= 0 && MSB >= LSB) ? LSB : 0;
constexpr int msb = (LSB >= 0 && MSB >= LSB) ? MSB : MemType::getWidth() - 1;

for (unsigned int ipage = 0; ipage < 4; ++ipage) {
for (unsigned int ipage = 0; ipage < memory_ref.getNPage(); ++ipage) {
//FIXME
for (unsigned int i = 0; i < memory_ref.getDepth()/4; ++i) {
for (unsigned int i = 0; i < memory_ref.getDepth(); ++i) {
const auto data_ref_raw = memory_ref.read_mem(ievt,i,ipage).raw();
const auto data_com_raw = memory.read_mem(ievt,i,ipage).raw();
const auto data_ref = data_ref_raw.range(msb,lsb);
Expand Down
11 changes: 5 additions & 6 deletions TrackletAlgorithm/MatchProcessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ template<TF::layerDisk Layer, TF::phiRegion PHI> constexpr uint32_t NPageSum();

#include "MatchProcessor_parameters.h"

template<regionType ASTYPE, regionType APTYPE, regionType VMSMEType, regionType FMTYPE, int maxFullMatchCopies, TF::layerDisk LAYER=TF::L1, TF::phiRegion PHISEC=TF::A>
template<regionType ASTYPE, regionType APTYPE, regionType VMSMEType, regionType FMTYPE, int maxFullMatchVariants, TF::layerDisk LAYER=TF::L1, TF::phiRegion PHISEC=TF::A>
void MatchCalculator(BXType bx,
ap_uint<1> newtracklet,
ap_uint<1>& isMatch,
Expand All @@ -913,7 +913,7 @@ void MatchCalculator(BXType bx,
const AllStubMemory<ASTYPE>* allstub,
const AllProjection<APTYPE>& proj,
ap_uint<VMStubMECMBase<VMSMEType>::kVMSMEIDSize> stubid,
FullMatchMemory<FMTYPE> fullmatch[maxFullMatchCopies]
FullMatchMemory<FMTYPE> fullmatch[maxFullMatchVariants]
){

#pragma HLS inline
Expand Down Expand Up @@ -1180,7 +1180,7 @@ constexpr unsigned kNbitsrzbinMPDisk = kNbitsrzbin + 1;

//////////////////////////////
// MatchProcessor
template<regionType PROJTYPE, regionType VMSMEType, unsigned kNbitsrzbinMP, regionType VMPTYPE, regionType ASTYPE, regionType FMTYPE, unsigned int nINMEM, int maxFullMatchCopies,
template<regionType PROJTYPE, regionType VMSMEType, unsigned kNbitsrzbinMP, regionType VMPTYPE, regionType ASTYPE, regionType FMTYPE, unsigned int nINMEM, int maxFullMatchVariants,
TF::layerDisk LAYER=TF::L1, TF::phiRegion PHISEC=TF::A>
void MatchProcessor(BXType bx,
// because Vivado HLS cannot synthesize an array of
Expand All @@ -1190,7 +1190,7 @@ void MatchProcessor(BXType bx,
const VMStubMEMemoryCM<VMSMEType, kNbitsrzbinMP, kNbitsphibin, kNMatchEngines>& instubdata,
const AllStubMemory<ASTYPE>* allstub,
BXType& bx_o,
FullMatchMemory<FMTYPE> fullmatch[maxFullMatchCopies]
FullMatchMemory<FMTYPE> fullmatch[maxFullMatchVariants]
){
#pragma HLS inline

Expand Down Expand Up @@ -1458,7 +1458,7 @@ void MatchProcessor(BXType bx,

if (hasMatch_save) {
isMatch = newtracklet_save ? ap_uint<1>(0) : isMatch;
MatchCalculator<ASTYPE, APTYPE, VMSMEType, FMTYPE, maxFullMatchCopies, LAYER, PHISEC>
MatchCalculator<ASTYPE, APTYPE, VMSMEType, FMTYPE, maxFullMatchVariants, LAYER, PHISEC>
(bx, newtracklet_save, isMatch, savedMatch, best_delta_z, best_delta_phi, best_delta_rphi, best_delta_r, allstub, allproj_save, stubindex_save,
fullmatch);
}
Expand All @@ -1483,7 +1483,6 @@ void MatchProcessor(BXType bx,
newtracklet_save = newtracklet;
allproj_save = allproj;
stubindex_save = stubindex;

} //end MC if


Expand Down
37 changes: 18 additions & 19 deletions TrackletAlgorithm/MemoryTemplate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,31 +31,35 @@ template<class DataType, unsigned int NBIT_BX, unsigned int NBIT_ADDR>
// (1<<NBIT_ADDR): depth of the memory for each BX
class MemoryTemplate
{
private:
#ifdef CMSSW_GIT_HASH
static constexpr unsigned int NBIT_BX = 0;
#endif

public:
static constexpr unsigned int DEPTH_BX = 1<<NBIT_BX;
static constexpr unsigned int DEPTH_ADDR = 1<<NBIT_ADDR;

typedef typename DataType::BitWidths BitWidths;
typedef ap_uint<NBIT_BX> BunchXingT;
typedef ap_uint<NBIT_ADDR> NEntryT;

protected:

DataType dataarray_[1<<NBIT_BX][1<<NBIT_ADDR]; // data array
NEntryT nentries_[1<<NBIT_BX]; // number of entries
DataType dataarray_[DEPTH_BX][DEPTH_ADDR]; // data array
NEntryT nentries_[DEPTH_BX]; // number of entries

public:

unsigned int getDepth() const {return (1<<NBIT_ADDR);}
unsigned int getNBX() const {return (1<<NBIT_BX);}
unsigned int getDepth() const {return DEPTH_ADDR;}
unsigned int getNBX() const {return DEPTH_BX;}

NEntryT getEntries(BunchXingT bx) const {
#pragma HLS ARRAY_PARTITION variable=nentries_ complete dim=0
return nentries_[bx];
}

const DataType (&get_mem() const)[1<<NBIT_BX][1<<NBIT_ADDR] {return dataarray_;}
const DataType (&get_mem() const)[DEPTH_BX][DEPTH_ADDR] {return dataarray_;}

DataType read_mem(BunchXingT ibx, ap_uint<NBIT_ADDR> index) const
{
Expand Down Expand Up @@ -108,7 +112,7 @@ class MemoryTemplate
#pragma HLS ARRAY_PARTITION variable=nentries_ complete dim=0
#pragma HLS inline
if(!NBIT_BX) ibx = 0;
if (addr_index < (1<<NBIT_ADDR)) {
if (addr_index < DEPTH_ADDR) {
#if defined __SYNTHESIS__ && !defined SYNTHESIS_TEST_BENCH
//The vhd memory implementation will write to the correct address!!
dataarray_[ibx][0] = data;
Expand All @@ -126,28 +130,23 @@ class MemoryTemplate
}
}

bool write_mem_new(BunchXingT ibx, DataType data, NEntryT addr_index)
bool write_mem_new(BunchXingT ibx, DataType data, ap_uint<1> overwrite)
{
#pragma HLS ARRAY_PARTITION variable=nentries_ complete dim=0
#pragma HLS inline
if(!NBIT_BX) ibx = 0;
if (addr_index < (1<<NBIT_ADDR)) {
//dataarray_[ibx][addr_index] = data;
if (nentries_[ibx] < DEPTH_ADDR) {
#if defined __SYNTHESIS__ && !defined SYNTHESIS_TEST_BENCH
//The vhd memory implementation will write to the correct address!!
dataarray_[ibx][0] = data;
#else
if(addr_index == 0) {
if(overwrite == 0) {
dataarray_[ibx][nentries_[ibx]++] = data;
} else {
dataarray_[ibx][nentries_[ibx]-1] = data;
}
#endif

#ifdef CMSSW_GIT_HASH
nentries_[ibx] = addr_index + 1;
#endif

return true;
} else {
return false;
Expand All @@ -166,9 +165,9 @@ class MemoryTemplate
void clear()
{
DataType data("0",16);
MEM_RST: for (size_t ibx=0; ibx<(1<<NBIT_BX); ++ibx) {
MEM_RST: for (size_t ibx=0; ibx<DEPTH_BX; ++ibx) {
nentries_[ibx] = 0;
for (size_t addr=0; addr<(1<<NBIT_ADDR); ++addr) {
for (size_t addr=0; addr<DEPTH_ADDR; ++addr) {
write_mem(ibx,data,addr);
}
}
Expand All @@ -185,7 +184,7 @@ class MemoryTemplate
return success;
}

bool write_mem(BunchXingT ibx, const std::string datastr, int base=16)
bool write_mem(BunchXingT ibx, const std::string& datastr, int base=16)
{
return write_mem(ibx, datastr.c_str(), base);
}
Expand All @@ -212,7 +211,7 @@ class MemoryTemplate

void print_mem() const
{
for (unsigned int ibx = 0; ibx < (1<<NBIT_BX); ++ibx) {
for (unsigned int ibx = 0; ibx < DEPTH_BX; ++ibx) {
for (unsigned int i = 0; i < nentries_[ibx]; ++i) {
edm::LogVerbatim("L1trackHLS") << ibx << " " << i << " ";
print_entry(ibx,i);
Expand Down
66 changes: 24 additions & 42 deletions TrackletAlgorithm/MemoryTemplateBinnedCM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
//way to implement this we will use this...
#include "SynthesisOptions.h"


#ifdef CMSSW_GIT_HASH
template<class DataType, unsigned int DUMMY, unsigned int NBIT_ADDR, unsigned int NBIT_BIN, unsigned int kNBitsphibinCM, unsigned int NCOPY>
#else
Expand All @@ -38,7 +37,6 @@ template<class DataType, unsigned int NBIT_BX, unsigned int NBIT_ADDR, unsigned
class MemoryTemplateBinnedCM{

private:

#ifdef CMSSW_GIT_HASH
static constexpr bool isCMSSW = true;
static constexpr unsigned int NBIT_BX = 0;
Expand All @@ -49,14 +47,18 @@ class MemoryTemplateBinnedCM{
#endif

public:
static constexpr unsigned int DEPTH_BX = 1<<NBIT_BX;
static constexpr unsigned int DEPTH_ADDR = 1<<NBIT_ADDR;
static constexpr unsigned int DEPTH_BIN = 1<<NBIT_BIN;

typedef ap_uint<NBIT_BX> BunchXingT;
typedef ap_uint<NBIT_ADDR-NBIT_BIN> NEntryT;

protected:
enum BitWidths {
kNBxBins = 1<<NBIT_BX,
kNSlots = 1<<NBIT_BIN,
kNMemDepth = 1<<NBIT_ADDR,
kNBxBins = DEPTH_BX,
kNSlots = DEPTH_BIN,
kNMemDepth = DEPTH_ADDR,
kNBitsRZBinCM = NBIT_BIN-kNBitsphibinCM,
kNBinsRZ = (1<<kNBitsRZBinCM),
slots = (1<<(NBIT_BX+NBIT_BIN-kNBitsphibinCM))
Expand Down Expand Up @@ -99,15 +101,14 @@ class MemoryTemplateBinnedCM{
return val;
}

const DataType (&getMem(unsigned int icopy) const)[1<<NBIT_BX][1<<NBIT_ADDR] {
const DataType (&getMem(unsigned int icopy) const)[DEPTH_BX][DEPTH_ADDR] {
#pragma HLS ARRAY_PARTITION variable=dataarray_ dim=1
if (isCMSSW) icopy = 0;
return dataarray_[icopy];
}


#ifndef CMSSW_GIT_HASH
const DataType (&get_mem() const)[NCOPY][1<<NBIT_BX][1<<NBIT_ADDR] {
const DataType (&get_mem() const)[NCOPY][DEPTH_BX][DEPTH_ADDR] {
return dataarray_;
}
#endif
Expand All @@ -118,13 +119,15 @@ class MemoryTemplateBinnedCM{
if (isCMSSW) {ibx = 0; icopy = 0;}
return dataarray_[icopy][ibx][index];
}

DataType read_mem(unsigned int icopy, BunchXingT ibx, ap_uint<NBIT_BIN> slot,
ap_uint<NBIT_ADDR> index) const {
#pragma HLS ARRAY_PARTITION variable=dataarray_ dim=1
// TODO: check if valid
if (isCMSSW) {ibx = 0; icopy = 0;}
return dataarray_[icopy][ibx][getNEntryPerBin()*slot+index];
}

bool write_mem(BunchXingT ibx, ap_uint<NBIT_BIN> slot, DataType data, unsigned int nentry_ibx) {
#pragma HLS ARRAY_PARTITION variable=dataarray_ dim=1
#pragma HLS ARRAY_PARTITION variable=binmask8_ complete dim=0
Expand All @@ -150,30 +153,18 @@ class MemoryTemplateBinnedCM{

if (nentry == ((1 << (NBIT_ADDR-NBIT_BIN)) - 1)) return false;

nentries_[ibx*kNBinsRZ+ibin].range(ireg*4+3,ireg*4)=nentry+1;
if (ibin!=0) {
nentries_[ibx*kNBinsRZ+ibin-1].range((ireg+8)*4+3,(ireg+8)*4)=nentry+1;
}
binmask8_[ibx][ibin].set_bit(ireg,true);
nentries_[ibx*kNBinsRZ+ibin].range(ireg*4+3,ireg*4)=nentry+1;
if (ibin!=0) {
nentries_[ibx*kNBinsRZ+ibin-1].range((ireg+8)*4+3,(ireg+8)*4)=nentry+1;
}
binmask8_[ibx][ibin].set_bit(ireg,true);

//icopy comparison must be signed int or future SW fails
writememloop:for (signed int icopy=0;icopy< (signed) NCP;icopy++) {
//icopy comparison must be signed int or future SW fails
writememloop:for (signed int icopy=0;icopy< (signed) NCP;icopy++) {
#pragma HLS unroll
dataarray_[icopy][ibx][getNEntryPerBin()*slot+nentry] = data;
}
dataarray_[icopy][ibx][getNEntryPerBin()*slot+nentry] = data;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomalin These changes just seem to affect the indentation, which was already a bit off, and now looks even more off, at least in my text editor. Could you clean this up, perhaps removing tabs altogether and consistently using two spaces to indent lines inside brackets?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the spaces in the HLS code are often created with "tab", and the number of spaces that "tab" corresponds to is not well-defined. It differs on different computers and in different editors/viewing tools. To fix this, we'd really have to globally replace all tabs by spaces, and then run clang to enforce a sensible indentation. And ask people to stop using tabs when editing the HLS code. But this should wait until there are no outstanding PRs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree, but I also think we can clean things up as we change other things. Especially with this block of code in particular, where the only changes that have been made are to the indentation, unless I've missed something.

Copy link
Contributor Author

@tomalin tomalin Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do this, but it would mean replacing all tabs with spaces in every line of every file where you've queried my changes to the indentation. And this of course would make it harder during your PR review to spot what I've changed. I suggest I make this change at the end, after I've answered all your non-indentation related comments?
P.S. There are other changes in this file, such as the introduction of "static constexpr unsigned int DEPTH_BX = 1<<NBIT_BX;".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, these indentation comments are the only ones left. The block highlighted here is the most significant example, since the only changes you made, AFAICT, are to the indentation. Some of those changes also don't make sense, like lines 156 and 160 being in different columns now.

I included suggestions for the other blocks I highlighted that you can simply accept if you like, so I hope these comments are fairly trivial to address.

#endif

#ifdef CMSSW_GIT_HASH
ap_uint<kNBitsRZBinCM> ibin;
ap_uint<kNBitsphibinCM> ireg;
(ireg,ibin)=slot;
nentries_[ibx*kNBinsRZ+ibin].range(ireg*4+3,ireg*4)=nentry_ibx+1;
if (ibin!=0) {
nentries_[ibx*kNBinsRZ+ibin-1].range((ireg+8)*4+3,(ireg+8)*4)=nentry_ibx+1;
}
binmask8_[ibx][ibin].set_bit(ireg,true);
#endif

return true;
}
else {
Expand All @@ -199,10 +190,10 @@ class MemoryTemplateBinnedCM{

DataType data("0",16);
for (size_t ibx=0; ibx<(kNBxBins); ++ibx) {
for (size_t icopy=0; icopy < NCOPY; icopy++) {
for (size_t ibin=0; ibin < kNMemDepth; ibin++) {
dataarray_[icopy][ibx][ibin] = data;
}
for (size_t icopy=0; icopy < NCP; icopy++) {
for (size_t ibin=0; ibin < kNMemDepth; ibin++) {
dataarray_[icopy][ibx][ibin] = data;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomalin Same comment regarding indentation.

Suggested change
for (size_t icopy=0; icopy < NCP; icopy++) {
for (size_t ibin=0; ibin < kNMemDepth; ibin++) {
dataarray_[icopy][ibx][ibin] = data;
}
for (size_t icopy=0; icopy < NCP; icopy++) {
for (size_t ibin=0; ibin < kNMemDepth; ibin++) {
dataarray_[icopy][ibx][ibin] = data;
}

}
// Clear nentries and binmask8
for (unsigned int ibin = 0; ibin < getNBins()/8; ++ibin) {
Expand Down Expand Up @@ -244,15 +235,6 @@ class MemoryTemplateBinnedCM{
DataType data(datastr.c_str(), base);

bool success = write_mem(ibx, slot, data, nentry_ibx);
#ifndef CMSSW_GIT_HASH
if (success) {
nentries_[ibx*kNBinsRZ+ibin].range(ireg*4+3,ireg*4)=nentry_ibx+1;
if (ibin!=0) {
nentries_[ibx*kNBinsRZ+ibin-1].range((ireg+8)*4+3,(ireg+8)*4)=nentry_ibx+1;
}
binmask8_[ibx][ibin].set_bit(ireg,true);
}
#endif

return success;
}
Expand Down
Loading