fifo_depth_optimization_flow details

[jmitrevs]

Description

I am opening this pull request more for discussion. If people agree with this change, the same change would also need to be made for the VivadoAccelator backend.

The issue is that the FifoDepthOptimization optimizer calls ModelGraph.write(), which executes the self._writer_flow as part of the optimizer, not part of the main flow. Therefore, there is no reason to do the writer flow beforehand, because it is executed specially nevertheless.

Also, concerning the + writer_pass, I don't think it should be there, because the write belongs in the hls_model.compile() step, which calls self.write() anyway, not in the convert_from_keras_model, which is where the FifoDepthOptimization step happens. The counterargument is that the stale model would still exist from the input to the FIFO depth optimization, but I would vote more for consistency, with the write happening in the compile step, not beforehand.

Type of change

• [x] Bug fix (non-breaking change that fixes an issue)

Tests

The main testing would be external to the standard pytests or compilations--basically to make sure that FIFO optimization still works. Do we currently run any FIFO depth optimization tests?

Checklist

• [x] I have read the guidelines for contributing.
• [x] I have commented my code, particularly in hard-to-understand areas.
• [x] I have made corresponding changes to the documentation.
• [x] My changes generate no new warnings.
• [x] I have added tests that prove my fix is effective or that my feature works.

[jmitrevs]

Another option, maybe better, is to split the fifo depth optimization into two optimizers, and not have the optimizer call write explicitly. Instead, the write can be explicitly scheduled in the flow between the two optimizers. Maybe this was the idea originally.

[thesps]

Hi, thanks for looking at this. My thoughts:

• I agree we don't need the writer flow in the requires, since it's the model with modified FIFO depths that we need to write.
• We put the + writer_pass at the end of the flow for the reason you put: to make sure that the stale project with the extra-large FIFOs is overwritten with the optimized one. If we decide that the flow shouldn't call the writer at the end, I think we should then add some cleanup of the stale project.
• Whether we split the flow into multiple passes: on this I have no preference. I think this is the only flow/pass that needs to write the project to do its transformation, and we just decided to have the pass call the write explicitly. If you'd prefer to manage that instead by having the flow broken down into passes, that works too.

Do we currently run any FIFO depth optimization tests?

We don't, but probably should when we can do Vivado-based tests on CERN Gitlab. I do have a test that I can run locally for this.

[jmitrevs]

I have no problem putting the + writer_pass bask. That's less effort than splitting the flow into multiple optimizers, though it does require #641 to function in all cases.

Splitting into multiple optimizers would make the flow something like:

fifo_depth_opt_passes = ['vivado:fifo_depth_optimization_pre'] + writer_pass +  \
          ['vivado:fifo_depth_optimization_post'] + writer_pass

and then there would be no need for an out-of-flow write(). Part of me thinks that this is a cleaner solution, though it is more work.

[jmitrevs]

I put back the + writer_pass, and made the corresponding change in vivaldo_accelerator. So provided the tests are good, this should be fine to accept (along with #641), but if others think putting the write explicitly in the flow is better, I can have a shot at the second option.

[jmitrevs]

@vloncar, I was curious if you have any thoughts about how the FIFO depth optmization starts the writer flow external to the main flow. Should it be updated to just work in a flow (i.e. split the fifo optimizer into two and explicitly schedule the writer within the flow--I have it more explicitly above), or is it fine as is, with the optimizer starting it's flow in the middle of the flow it is part of? The latter is why I created #641, so that if you interrupt a flow you still have current information.

[jmitrevs]

I think this is a fairly minor big fix: basically making the fifo depth optimization depend on vivado:ip and not the writer. It is more of an optimization since it removes a write pass that is completely overwritten when doing the fifo depth optimization. I think it should be merged, therefore. The issue of whether we rework the fifo depth optimization or not we can discuss later.

[jmitrevs]

I decided to move the discussion about potentially splitting the optimizer to #705, and in this pull request I just fix the requires to depend on the ip flow, not the writer flow, as discussed earlier. Therefore, I think this is a fairly straightforward PR that I suggest we merge, provided the tests succeed.

The precommit hooks required that I make more changes to the vivadoaccelator backend, but the real change is the same as in the vivado backend. (I think the backends can both be cleaned up, and path navigation could potentially be made with a context manager, but that's beyond the scope of this PR.)

[jmitrevs]

Can people look at this? It's a minor change that has been sitting for a while.