yosys-f4pga-plugins icon indicating copy to clipboard operation
yosys-f4pga-plugins copied to clipboard

UHDM Plugin: Very slow string comparison

Open QuantamHD opened this issue 2 years ago • 4 comments

Nearly 11% of the profile in the UHDM plugin is running this block of code.

https://github.com/chipsalliance/yosys-f4pga-plugins/blob/main/systemverilog-plugin/UhdmAst.cc#L1200-L1202

Is there something we can do to speed this up?

  • Some of my thoughts are can we create a temporary hash_map for AST::Nodes and their children
  • Can we compare a cheaper property than its name. Maybe it's type first?

image

QuantamHD avatar Apr 06 '22 20:04 QuantamHD

@kamilrakoczy Can you take a look, or suggest a path forward. I can send a PR

QuantamHD avatar Apr 07 '22 15:04 QuantamHD

Both of these ideas make sense to us. Comparing the type as well as the name is a simple fix, so it's definitely worth trying. A temporary hash map would probably work better here, though we should keep an eye on memory usage for bigger modules.

If you're interested in a completely different solution:

This function should become obsolete if we switched to the elaborated model in UHDM.

If we used the elaborated model, each module instance would only get processed once. Thus removing the need for replacing any of their children.

It would also significantly simplify the module handling code.

I was experimenting with it for a bit, but ran into many issues. Some of our assumptions in the frontend no longer held up when using the elaborated model.

You can take a look at my (fairly stale) branch: https://github.com/antmicro/yosys-f4pga-plugins/commits/kbieganski/uhdm-plugin/elaborated-model

(Remember to use the -elabuhdm flag in Surelog for this to work in any way) The main changes are

  • https://github.com/antmicro/yosys-f4pga-plugins/commit/7b8794febaecc827b973499789785fdb9df7c071#diff-0de03cd2799cd71f0b7bb897fe3c50a057d26a822810348b9bdb4acf24e9cdfbL1283-R1286
  • https://github.com/antmicro/yosys-f4pga-plugins/commit/7b8794febaecc827b973499789785fdb9df7c071#diff-0de03cd2799cd71f0b7bb897fe3c50a057d26a822810348b9bdb4acf24e9cdfbL1354-R1465

Instead of going through uhdmallModules and then through uhdmtopModules and replacing nodes, we only iterate over uhdmtopModules. So we no longer worry about 'partial' modules that we clone and fill in. We simply create a new module for each instance (of course if a module doesn't use parameters, this is unnecessary; I think reusing parameterless modules shouldn't be an issue).

The rest is a consequence of those changes: an attempt to fix memories, more complex handling of vpiRefObj as the elaborated model sometimes actually puts operations in place of those (it happens for assignment patterns IIRC). With new versions of Surelog these parts may be completely outdated.

Feel free to take over, as I won't be able to continue this any time soon.

kbieganski avatar Apr 08 '22 08:04 kbieganski

Is it ever the case that we would want to replace a child whose name is the same, but type is different?

QuantamHD avatar Apr 08 '22 23:04 QuantamHD

Is it ever the case that we would want to replace a child whose name is the same, but type is different?

I don't think there is a case like this at the moment. There used to be one when we were replacing parameter and localparam nodes, but it seems to be resolved now in UHDM.

rkapuscik avatar Apr 12 '22 12:04 rkapuscik