Templates vs. arguments

[aehart]

To test whether we need to use template parameters in the definitions of our processing modules, as we have assumed so far, I reorganized the TC so that the current template parameters in the top-level template function are passed as function arguments instead. Compare the top-level template function on the master branch:

firmware-hls/TrackletAlgorithm/TrackletCalculator.h

Lines 186 to 197 in 88e4783

	template<TF::seed Seed, TC::itc iTC, uint8_t NSPMem> void
	TrackletCalculator(
	const BXType bx,
	const AllStubMemory<InnerRegion<Seed>()> innerStubs[],
	const AllStubMemory<OuterRegion<Seed>()> outerStubs[],
	const StubPairMemory stubPairs[],
	BXType& bx_o,
	TrackletParameterMemory * const trackletParameters,
	TrackletProjectionMemory<BARRELPS> projout_barrel_ps[],
	TrackletProjectionMemory<BARREL2S> projout_barrel_2s[],
	TrackletProjectionMemory<DISK> projout_disk[]
	);

with that on the template_purge branch:

firmware-hls/TrackletAlgorithm/TrackletCalculator.h

Lines 193 to 208 in 5d8b1b3

	template<regionType InnerRegion, regionType OuterRegion> void
	TrackletCalculator(
	const TF::seed Seed,
	const TC::itc iTC,
	const uint8_t NSPMem,
	const BXType bx,
	const AllStubMemory<InnerRegion> innerStubs[],
	const AllStubMemory<OuterRegion> outerStubs[],
	const StubPairMemory stubPairs[],
	BXType& bx_o,
	TrackletParameterMemory * const trackletParameters,
	TrackletProjectionMemory<BARRELPS> projout_barrel_ps[],
	TrackletProjectionMemory<BARREL2S> projout_barrel_2s[],
	TrackletProjectionMemory<DISK> projout_disk[]
	);

In making this change, I had to introduce new template parameters for the inner and outer region types, since these are passed as parameters to the memory classes that define our interfaces. I don't see an obvious way around this. The inner and outer region types, however, do not change with each instance of the TC as the previous set of parameters did (e.g., all L1L2 TCs will have InnerRegion = BARRELPS and OuterRegion = BARRELPS, etc.). An alternative to this would be to keep just the seed as the only template parameter, and determine the inner and outer region types from it, as is currently done on the master branch.

Below is a comparison of the timing and resource utilization, all from the post-implementation reports, for a VU7P. The version on the template_purge branch has slightly worse timing and some of the resources shifted around, but overall it seems to be very similar to the version on the master branch.

	master	template_purge
minimum clock period	3.706 ns	3.760 ns
CLB	423	474
LUT	1814	1790
FF	1981	1960
DSP	73	73
BRAM	5	5
SRL	222	222

I think this demonstrates that we can convert most of our template parameters to function arguments. This doesn't necessarily change anything about how we export IP cores for the firmware, but using function arguments might be more convenient for the HLS-based emulation.

[aehart]

@blwiner since it seems we can convert most of the compile-time parameters to function arguments, which can be determined at run-time, the question now is whether the emulation would benefit from this change.

[blwiner]

@aehart thanks for looking into this. I should talk more with the others developing on the emulation side. Perhaps this could be a discussion point at the HLS Chat this week?

[aehart]

Perhaps this could be a discussion point at the HLS Chat this week?

Yes, I think we can do this.

[aehart]

Here is a version of the TrackletCalculator without any template parameters:

firmware-hls/TrackletAlgorithm/TrackletCalculator.h

Lines 210 to 259 in 17f4ef3

	void
	TrackletCalculator(
	const TF::seed Seed,
	const TC::itc iTC,
	const uint8_t NSPMem,
	const BXType bx,
	const AllStubMemory<BARRELPS> innerStubs[],
	const AllStubMemory<BARRELPS> outerStubs[],
	const StubPairMemory stubPairs[],
	BXType& bx_o,
	TrackletParameterMemory * const trackletParameters,
	TrackletProjectionMemory<BARRELPS> projout_barrel_ps[],
	TrackletProjectionMemory<BARREL2S> projout_barrel_2s[],
	TrackletProjectionMemory<DISK> projout_disk[]
	);
	void
	TrackletCalculator(
	const TF::seed Seed,
	const TC::itc iTC,
	const uint8_t NSPMem,
	const BXType bx,
	const AllStubMemory<BARRELPS> innerStubs[],
	const AllStubMemory<BARREL2S> outerStubs[],
	const StubPairMemory stubPairs[],
	BXType& bx_o,
	TrackletParameterMemory * const trackletParameters,
	TrackletProjectionMemory<BARRELPS> projout_barrel_ps[],
	TrackletProjectionMemory<BARREL2S> projout_barrel_2s[],
	TrackletProjectionMemory<DISK> projout_disk[]
	);
	void
	TrackletCalculator(
	const TF::seed Seed,
	const TC::itc iTC,
	const uint8_t NSPMem,
	const BXType bx,
	const AllStubMemory<BARREL2S> innerStubs[],
	const AllStubMemory<BARREL2S> outerStubs[],
	const StubPairMemory stubPairs[],
	BXType& bx_o,
	TrackletParameterMemory * const trackletParameters,
	TrackletProjectionMemory<BARRELPS> projout_barrel_ps[],
	TrackletProjectionMemory<BARREL2S> projout_barrel_2s[],
	TrackletProjectionMemory<DISK> projout_disk[]
	);

It's overloaded to accommodate the different possible types of inner and outer stubs. I think that means the types of the corresponding memories need to be determined at build-time, so that the compiler knows which version to call. Not sure how much of a limitation that is, but we could also pass the memory arrays as generic pointers (say, void *) and cast them as the appropriate memory type based on the value of Seed.

[pwittich]

from a code style point of view pointer to void sounds error-prone and probably not a great idea from safety either.

[aehart]

@pwittich I think I agree with that. And I'm not even sure it would pass muster with the CMSSW code guidelines. A better solution would probably be to define a dummy MemoryTemplateBase class, which would be the grandparent class for all memories, and pass a pointer to that class instead of void *. I think then you could even use dynamic_cast, which does some type safety checks.

In any case, it should be possible to define a generic function interface for the emulation, if the emulation folks think that would be useful.

[EmyrClement]

@aehart I think your suggestion of a dummy MemoryTemplateBase may also help in how we store the various memories in the CMSSW event. At the moment, taking the AllStubMemory that are written out by the VMR as an example, to store the different templated versions of these memories in the same vector we make use of std::variant and store objects like :

typedef std::vector<std::variant< AllStubMemory<BARRELPS>, AllStubMemory<BARREL2S>, AllStubMemory<DISK> >> ASMemories;

But I think I would prefer something simpler like storing std::vector<MemoryTemplateBase> (or possibly just the underlying ap_uint data?) and also some information on how to read the data (or how to cast it to the correct templated type).

The use of std::variant had worked well for us so far, but fails to compile in the recent CMSSW versions because we are trying to store an std::variant in the CMSSW event (the gcc version changed so is likely part of the cause). I hope this can be solved, but am also curious if we can remove the use of std::variant as well.

[aehart]

If we did introduce a common base class, that would allow you to create a common vector containing pointers to all the memories in the project (or just the memories of a certain template class like AllStubMemory). But I don't think you could create a std::vector<MemoryTemplateBase>, or of just the underlying ap_uint data, because the size of the objects is not necessarily the same between AllStubMemory<BARRELPS>, AllStubMemory<BARREL2S>, etc. That's not a problem for pointers, since they're all the same size, and you just have to satisfy the type safety requirements of C++.

Another potential option for you is boost::variant. CMSSW supports boost as an external tool, and I've used it in the past. I've never tried to store it in the CMSSW event, so I'm not sure if that will work any better than std::variant, but it might be worth a try.