cms-L1TK / firmware-hls

HLS implementation of the tracklet pattern reco modules of the Hybrid tracking chain
15 stars 24 forks source link

Move lookup tables outside of modules for every module #142

Open pwittich opened 3 years ago

pwittich commented 3 years ago

Not all modules have external LUTs. We should move to external LUTs for all modules.

With internal LUTs:

can we move these to external LUTs to achieve uniformity across the project and simplify the integration with the emulation/CMSSW?

aehart commented 3 years ago

For the TE, external LUTs make sense because it greatly reduces the number of IPs that we have to export.

But for the TC, and maybe for other modules, I think the only thing we gain with external LUTs is a more complicated interface.

Maybe the rule should be to have internal LUTs unless there is a good reason to move them out of the module.

pwittich commented 3 years ago

maybe others can respond but the motivation was for making the interface to the emulation/cmssw more uniform. @blwiner can you weigh in

aehart commented 3 years ago

@EmyrClement since you raised this question in your presentation last week, can you comment on whether there is a preference from the emulation?

EmyrClement commented 3 years ago

The benefit of internal LUTs is that the CMSSW emulation doesn't need to work out which LUTs are required for a particular instance of a processing module - i.e. this work would already have been done somewhere else, and makes the CMSSW-HLS interface somewhat simpler. Though the CMSSW emulation is (or is almost) doing this, and is essentially the same task as accessing the wiring map, so is not really an issue.

I think the downside to internal LUTs is if that updating them in the emulation is a longer process. I guess this shouldn't happen often, but even for a small tweak this would mean deriving the LUTs and writing to file, compiling the HLS with the new LUTs (I guess similar so far to what you would need to do to synthesize the HLS with new LUTs), and then updating the external library in CMSSW that corresponds to our HLS. The last step would be a significant change in the central CMSSW repository, which we should avoid doing too often. For external LUTs, we would only need to rederive the LUTs by changing a python configuration parameter. If the LUTs are guaranteed to almost never change once they are finalized, this isn't really an issue, but this still makes me favour external LUTs.

aehart commented 3 years ago

I guess I hadn't realized that our HLS code would be compiled as an external library in CMSSW, in which case, I think you make a really good point. But what's the reason for this? I had assumed the HLS code would eventually become part of an ordinary package in CMSSW.

EmyrClement commented 3 years ago

A few years ago there was some discussion with the central software group about how to use HLS code within CMS, and the outcome was that it would be best to include it as an external library. I wasn't part of that discussion, so unfortunately don't know the details, but that's the way we are working towards at the moment with the emulation.

aehart commented 3 years ago

OK fair enough. Then the solution I'm leaning toward at the moment is to essentially have two layers of top functions for each module. In pseudocode, that might look something like the following:

FW_TopFunction(…) // no lookup tables as arguments
{
  // any lookup tables are initialized here

  SW_TopFunction(table_0, table_1, …);
}

The SW_TopFunction is the function that would be compiled and included in the external library for CMSSW. It takes in any lookup tables as function arguments. And the FW_TopFunction is what we would use for C-synthesis and would not take lookup tables as arguments. This way we get the best of both worlds: the lookup tables are configurable at run-time via Python in the emulation, but they remain internal to the IP cores that HLS exports so that the interfaces aren't needlessly complicated.

The only exception to this, I think, would be the TrackletEngine, where we want the top function that is used for C-synthesis to take lookup tables as arguments. But with this proposal, that would only be an exception for the firmware and not the emulation. All of the emulation top functions for modules with lookup tables would take them as arguments.

Does that make sense?

blwiner commented 3 years ago

We might need to think about this a little more on the emulation side. Right now we compile the top function but what actually gets used in CMSSW is the templated lower level function, not the top function. (At least this is the case for the MatchProcessor.) Maybe this is something we need to revisit, but I not yet convinced what you propose is easy to do on the emulation side. I certainly need to think about it a little more.

aehart commented 3 years ago

The call to SW_TopFunction in the code above could very well be a call to a templated function. The point is that it needs to be something that can be fully compiled and included in an external library. A non-templated function trivially fits that bill, but an instance of a templated function, with the templates fully defined, would also work.

BTW, I added a slot to the HLS chat tomorrow to discuss this and issue #145, with the goal of laying out the possibilities and determining the needs of the emulation.

pwittich commented 3 years ago

it would be nice to decide what to do here soon so we can close this issue.