Skip to content

Override parent backend optimizer passes with derived backend passes #597

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.

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

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

thesps
Copy link
Contributor

@thesps thesps commented Jul 13, 2022

Opening this PR for discussion, with the solution if agreed.

In #509 we noticed that if a base and derived Backend class both define an optimizer pass of the same name, it is the base backend pass that remains registered to the derived backend set of passes. This is a bit counterintuitive - I think you'd expect the derived class to override passes in the same way it might override methods. I put the change in a separate branch since it touches something deeper in the framework.

A concrete example from #509:

print(hls4ml.model.optimizer.get_optimizer('vivadoaccelerator:fifo_depth_optimization'))
<hls4ml.backends.vivado.passes.fifo_depth_optimization.FifoDepthOptimization object at 0x7f6f353b5820>

Notice I get the pass registered to vivadoaccelerator backend, which returns an object in hls4ml.backends.vivado.

With this PR I changed the behaviour to register the base class passes first, which are then updated with the current class.
So now it yields:

print(hls4ml.model.optimizer.get_optimizer('vivadoaccelerator:fifo_depth_optimization'))
<hls4ml.backends.vivado_accelerator.passes.fifo_depth_optimization.FifoDepthOptimization object at 0x7f35d77f65e0>

As expected, getting the pass from vivadoaccelerator returns the pass from hls4ml.backends.vivado_accelerator.

I think this case is our only example of the derived class defining a pass of the same name as the parent class. The backends that derive from FPGABackend only add passes, they don't override (I think).

@thesps thesps requested a review from bo3z July 18, 2022 11:52
@thesps
Copy link
Contributor Author

thesps commented Jul 18, 2022

@bo3z since you added this optimizer pass inheritance for #523, maybe you can take a look if this makes sense

@bo3z
Copy link
Contributor

bo3z commented Jul 18, 2022

Looks good to me. Although it seems that one of the tests fails.

This approach could be further extended by overriding from FPGABackend, instead of simply adding the passes. Some passes are nearly identical for Quartus and Vivado (and I expect many more to be so, as more layers and streaming I/O start being supported on Quartus) - in this case, we only add them. There are, however, a few passes that only differ in one variable, e.g. the backend name, such as quantization_templates.py. This is essentially code duplication, so what we could do is something along the lines of:

  • Move common / nearly identical passes to base module and declare as abstract classes, so they can never be instantiated
  • Passes identical to all backends, should be fully declared in the base module and simply inherit in the child backends
  • Passes with near-identical functionality, such as quantization_templates.py should still be declared (as much as possible), in the base module, and then in the child modules overwrite the variables/functions that are backend-specific.

@thesps
Copy link
Contributor Author

thesps commented Jul 18, 2022

Although it seems that one of the tests fails.

I think it's unrelated to this, but to #602. I'm happy to wait until that's resolved in any case.

I think all that you added makes sense, and then I think this PR is needed to get the intended inheritance behaviour

@thesps thesps force-pushed the derived_backend_passes branch from eb950fc to 4a98f2a Compare July 18, 2022 13:23
@bo3z
Copy link
Contributor

bo3z commented Jul 19, 2022

Although it seems that one of the tests fails.

I think it's unrelated to this, but to #602. I'm happy to wait until that's resolved in any case.

I think all that you added makes sense, and then I think this PR is needed to get the intended inheritance behaviour

Sounds good to me, happy to merge this.

@vloncar vloncar merged commit a3cb4de into main Jul 19, 2022
@jmduarte jmduarte deleted the derived_backend_passes branch November 2, 2022 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants