Skip to content
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

GarNet import of hls_math not working #365

Closed
thaarres opened this issue Jul 21, 2021 · 10 comments
Closed

GarNet import of hls_math not working #365

thaarres opened this issue Jul 21, 2021 · 10 comments
Labels

Comments

@thaarres
Copy link
Contributor

Hi @vloncar @yiiyama @jmduarte !
I’m trying to synthesize a simple GarNet model (one GarNet layer) using the branch this branch. I get an error related to an import of hls_math :

Writing HLS project
Done
In file included from firmware/parameters.h:16:0,
                 from firmware/myproject.cpp:22:
firmware/nnet_utils/nnet_garnet.h:25:22: fatal error: hls_math.h: No such file or directory
 #include "hls_math.h"

The hls math header is included by the garnet layer template
here
and is used to compute an exponential.

Could this header file be added to the project or the exponential replaced by a look-up table?

@thesps
Copy link
Contributor

thesps commented Jul 21, 2021

Do you see this when trying to hls_model.compile() or hls_model.build()?

@thaarres
Copy link
Contributor Author

In hls_model.compile()

@thesps
Copy link
Contributor

thesps commented Jul 21, 2021

Could you try hls_model.build() ? I think it's because the hls_math.h header file is not copied here. But it should work when you use build, i.e. compile it with Vivado. Then you'll need to write the test data to a file and use csim to test. But you can still do it all from Python

@thaarres
Copy link
Contributor Author

As a quick fix suggested by @vloncar , I am currently pointing the compiler to the Vivado headers (which works). But @vloncar suggested there might be licensing issues with including the header file as a permanent solution

@yiiyama
Copy link
Contributor

yiiyama commented Aug 12, 2021

Sorry @thaarres somehow GitHub didn't send me a notification when you mentioned me above or I accidentally deleted the email - anyway I only saw this issue today.

How about adding a compiler flag like -DNO_VIVADO in build_lib.sh? Then I can switch the include in nnet_garnet between hls_math.h and <cmath>. What do people think about this?

yiiyama added a commit to yiiyama/hls4ml that referenced this issue Aug 12, 2021
@yiiyama
Copy link
Contributor

yiiyama commented Aug 13, 2021

I included this -DNO_VIVADO fix in #344

@vloncar
Copy link
Contributor

vloncar commented Aug 13, 2021

With -DNO_VIVADO we will get different results from python simulation (the compile() feature) and the C simulation/synthesis from Vivado. In other layers we use the lookup tables for expensive math functions, is there a reason that won't work here?

@yiiyama
Copy link
Contributor

yiiyama commented Aug 13, 2021

hls_math.h is used for hls::exp() in this code, and the function is called to fill a lookup table, filling controlled by a static boolean flag. So I think the usage pattern is exactly the same as other layers. The reason I didn't use std::exp() was that I saw this in the Vivado reference:

If the HLS math library is used in the C source code, there will be no difference between the C simulation and the C/RTL co-simulation. A C simulation using the HLS math library, may however differ from a C simulation using the standard C math library.

But now that I think about it,

  • Because the function call is not in run time, maybe we do get identical results between C sim and co-sim even with std::exp()?
  • Even if we don't, I'm OK with using std::exp() if that's preferable compared to -DNO_VIVADO.

@jmduarte
Copy link
Member

Re: #365 (comment) See #344 (comment)_ for a followup on python API vs csim differences with the current approach.

I agree it would be nice to guarantee these two agree.

@yiiyama it'd be interesting to see if we can just use std::exp() to fill the lookup table and preserve python/csim/cosim matching? Alternatively maybe the issue is just coming from the cast to double before std::exp()?: https://github.com/fastmachinelearning/hls4ml/pull/344/files#r688860255

@jmduarte jmduarte added the bug label Sep 12, 2021
jmduarte pushed a commit that referenced this issue Oct 11, 2021
jmduarte pushed a commit that referenced this issue Oct 12, 2021
@jmduarte
Copy link
Member

jmduarte commented Apr 9, 2023

Fixed by #344

@jmduarte jmduarte closed this as completed Apr 9, 2023
calad0i pushed a commit to calad0i/hls4ml that referenced this issue Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants