fix nondefault project name handling #626

jmitrevs · 2022-08-01T22:45:35Z

Description

As discussed in #624, convert_from_keras_model with project_name set to nondefault value (e.g. mynewprojname) fails to build, because in build.tcl the variable ${mynewprojname} is not defined. This fixes the problem my always leaving the variable called myproject, but just changing the value of the variable. Some changes in the tcl code were requried since now since the variable name does not always match the value it contains. The build.tcl indentation was fixed, and the parsing of the Vivado report was fixed. Note that this is broken in main even in the default case currently.

Type of change

Bug fix (non-breaking change that fixes an issue)

Tests

Testing was done offline by modifying and running part1 of the hls4ml-tutorial, as mentioned in #624. Nevertheless, we probably should add a test. The complication is that the test requires synthesis, and generally our pytests don't do synthesis. Plus, synthesis is long. Do we want to add another synthesis test for this case or modify an existing one?

Checklist

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

…nly as a variable

jmitrevs · 2022-08-02T11:53:40Z

The qkeras pytest failure is a known problem that exists in the main branch even without this change.

thesps · 2022-08-02T11:59:45Z

Thanks for the fix, I don't think we need a new test for this

hls4ml/templates/vivado/build_prj.tcl

hls4ml/report/vivado_report.py

…tion name to better match

hls4ml/writer/vivado_writer.py

jmitrevs · 2022-08-03T19:34:16Z

The last push was not a complete fix. I'll send a message again when I think it's ready.

jmitrevs · 2022-08-03T21:10:40Z

I still need to test vsynth=True and also that I didn't mess up the FIFO optimizations.

jmitrevs · 2022-08-03T21:40:20Z

Actually, looking in the main branch as a starting point, I see

 'VivadoSynthReport': {'LUT': '0',
  'FF': '0',
  'BRAM_18K': '0',
  'URAM': '0',
  'DSP48E': '0'}

when I do part1 of the hls4ml-tutorial with hls_model.build(vsynth=True) so vsynth might be broken in the main branch. Will try to investigate.

vloncar · 2022-08-03T21:51:20Z

This has been broken for some time now, I observed it as well while playing with Vitis. Thanks for looking into it.

jmitrevs · 2022-08-03T23:14:45Z

This seems to break the VivadoAccelerator backend.

jmitrevs · 2022-08-16T15:57:31Z

I believe this is ready for merging.

…". The commit breaks the Vivado Accelerator workflow, and the fix is unclear to me right now. This reverts commit e8f048a.

This reverts commit e8f048a.

add new files for conv1dtranspose resource clean up so that conv code is reached. Still need to get the actual implementation matching keras implement conv1dtranspose super inefficiently (gets correct answer though) try to fix indices to make code work make the c code work for conv1dtranspose reduce weight dimensions to properly reflect transposed kernel size clean up so that transpose filter width is passes around from config fix code such that simple transpose layer gets synthesized move variables out of loops, optimize slightly and add in alternative method of computation to compute by kernel (that option is not optimized as of now) add in conv1d transpose linebuffer format code. seems to work, unsure of if it is optimized yet trying to fix stream behavior get transpose compilation working mostly as expected. weird jump in latency from reuse 1 to 2 still exists initial conv2dtranspose addition. Output is permuted as of now. output in correct order. using large array to buffer output though fix up conv1dtranspose a bit to pad correctly. fix up stream instructions for both 1d and 2d transposes fix allowed reuse factors for transpose layers update to new conv methods for io_parallel. Still some issues with multiple filters as well as some padding issues clean up error with multiple filters and larger kernels optimize conv transpose resource to get it working reasonably well. may still have slight optimization left fix output to conv1d transpose resource add conv2dtranspose io_parallel implementation. Can still be optimized small changeup to data storage in conv1d parallel fix zero padding pass addition for transpose stream layers move transposing of weight matrix to resource_strategy for transpose layers change how stream loads in weights to be like parallel for conv transposes. unroll all stride steps completely fix output of 1d transpose parallel to be faster change 1d transpose weight input to be 2-dimensional (passed from python code) change 2d transpose weight input to be 3-dimensional (passed from python code) small changes to transposes Revert "fix nondefault project name handling (fastmachinelearning#626)". The commit breaks the Vivado Accelerator workflow, and the fix is unclear to me right now. This reverts commit e8f048a. steps towards getting integer inputs to work

* only rename value of ${myproject}, not the variable name * consistently indent build_prj.tcl; be consistent in using myproject only as a variable * fix parsing of Vivado reports * migrate more to using variables instead of changing text; change function name to better match * rename myproject to project_name in remaining Vivado places * try to fix vivado accelerator for project_name * replace forgotten myproject with project_name * fix FIFO depth optimization tcl

add new files for conv1dtranspose resource clean up so that conv code is reached. Still need to get the actual implementation matching keras implement conv1dtranspose super inefficiently (gets correct answer though) try to fix indices to make code work make the c code work for conv1dtranspose reduce weight dimensions to properly reflect transposed kernel size clean up so that transpose filter width is passes around from config fix code such that simple transpose layer gets synthesized move variables out of loops, optimize slightly and add in alternative method of computation to compute by kernel (that option is not optimized as of now) add in conv1d transpose linebuffer format code. seems to work, unsure of if it is optimized yet trying to fix stream behavior get transpose compilation working mostly as expected. weird jump in latency from reuse 1 to 2 still exists initial conv2dtranspose addition. Output is permuted as of now. output in correct order. using large array to buffer output though fix up conv1dtranspose a bit to pad correctly. fix up stream instructions for both 1d and 2d transposes fix allowed reuse factors for transpose layers update to new conv methods for io_parallel. Still some issues with multiple filters as well as some padding issues clean up error with multiple filters and larger kernels optimize conv transpose resource to get it working reasonably well. may still have slight optimization left fix output to conv1d transpose resource add conv2dtranspose io_parallel implementation. Can still be optimized small changeup to data storage in conv1d parallel fix zero padding pass addition for transpose stream layers move transposing of weight matrix to resource_strategy for transpose layers change how stream loads in weights to be like parallel for conv transposes. unroll all stride steps completely fix output of 1d transpose parallel to be faster change 1d transpose weight input to be 2-dimensional (passed from python code) change 2d transpose weight input to be 3-dimensional (passed from python code) small changes to transposes Revert "fix nondefault project name handling (#626)". The commit breaks the Vivado Accelerator workflow, and the fix is unclear to me right now. This reverts commit e8f048a. steps towards getting integer inputs to work

jmitrevs added 3 commits August 1, 2022 13:27

only rename value of ${myproject}, not the variable name

c1b7ead

consistently indent build_prj.tcl; be consistent in using myproject o…

d1a6557

…nly as a variable

fix parsing of Vivado reports

0f8cd26

thesps approved these changes Aug 2, 2022

View reviewed changes

vloncar reviewed Aug 2, 2022

View reviewed changes

hls4ml/templates/vivado/build_prj.tcl Outdated Show resolved Hide resolved

vloncar reviewed Aug 2, 2022

View reviewed changes

hls4ml/report/vivado_report.py Outdated Show resolved Hide resolved

migrate more to using variables instead of changing text; change func…

c7feda3

…tion name to better match

vloncar reviewed Aug 3, 2022

View reviewed changes

hls4ml/writer/vivado_writer.py Outdated Show resolved Hide resolved

rename myproject to project_name in remaining Vivado places

c5980ba

jmitrevs added 3 commits August 4, 2022 17:29

try to fix vivado accelerator for project_name

156341c

replace forgotten myproject with project_name

a1a6454

fix FIFO depth optimization tcl

5b3928d

jmitrevs mentioned this pull request Aug 16, 2022

Fix parsing of logic synthesis reports #639

Merged

1 task

vloncar approved these changes Aug 16, 2022

View reviewed changes

vloncar merged commit e8f048a into main Aug 16, 2022

judicaelclair added a commit to judicaelclair/hls4ml that referenced this pull request Oct 19, 2022

Revert "fix nondefault project name handling (fastmachinelearning#626)"

8f798db

This reverts commit e8f048a.

jmduarte deleted the jmitrevs/fix-project-name branch November 2, 2022 02:30

vloncar mentioned this pull request Dec 14, 2022

Fix function call in Alveo tcl script #694

Merged

7 tasks

jmduarte mentioned this pull request Mar 18, 2023

attempt to add support for conv1d transpose jmduarte/hls4ml#18

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix nondefault project name handling #626

fix nondefault project name handling #626

jmitrevs commented Aug 1, 2022

jmitrevs commented Aug 2, 2022

thesps commented Aug 2, 2022

jmitrevs commented Aug 3, 2022

jmitrevs commented Aug 3, 2022

jmitrevs commented Aug 3, 2022

vloncar commented Aug 3, 2022

jmitrevs commented Aug 3, 2022

jmitrevs commented Aug 16, 2022

fix nondefault project name handling #626

fix nondefault project name handling #626

Conversation

jmitrevs commented Aug 1, 2022

Description

Type of change

Tests

Checklist

jmitrevs commented Aug 2, 2022

thesps commented Aug 2, 2022

jmitrevs commented Aug 3, 2022

jmitrevs commented Aug 3, 2022

jmitrevs commented Aug 3, 2022

vloncar commented Aug 3, 2022

jmitrevs commented Aug 3, 2022

jmitrevs commented Aug 16, 2022