cancel
Showing results for 
Show  only  | Search instead for 
Did you mean: 
Highlighted
Adventurer
Adventurer
665 Views
Registered: ‎02-16-2017

vitis_hls breaks stream depth pragma

Jump to solution

Hi there,

I maintain the open source library hlslib, which provides a more feature-rich interface to hls::stream through the hlslib::Stream class.
This relies on being able to use #pragma HLS STREAM to set the depth of the underlying hls::stream object from the hlslib::Stream class.

Starting from Vitis 2020.1, the default HLS tool used by the accelerator/SDAccel flow has been changed from vivado_hls to vitis_hls.
This has broken the STREAM pragma when called from within a constructor, which in turn has broken hlslib::Stream.

When calling #pragma HLS STREAM depth=N for any value of N, vitis_hls sets the depth of the stream to a (seemingly unrelated) huge number (if I set 128, I get 111934792; if I set 256, I get 86637896; if I set 512, I get 104049992. Don't see any pattern here).
Running the exact same code through vivado_hls from Vivado 2020.1 sets the stream depth to the correct value.

I've pasted a minimal example of a kernel file that produces this bug (I was not allowed to attach this) and attached a script to synthesize it, which can be run with vivado_hls -f Synthesis.tcl (will produce a stream of depth 128), or with vitis_hls -f Synthesis.tcl (will produce a stream of depth 111934792).

Any insight into what could be causing this, or into how vivado_hls and vitis_hls differ, would be greatly appreciated!

#include <hls_stream.h>

template <typename T>
class Stream {

public:

  Stream(unsigned long val) {
    #pragma HLS INLINE
    #pragma HLS STREAM variable=stream_ depth=val
  }

  T Pop() {
    #pragma HLS INLINE
    return stream_.read();
  }

  void Push(T const &val) {
    #pragma HLS INLINE
    stream_.write(val);
  }

private:
  
  hls::stream<T> stream_;
};

void Read(int const *memory, Stream<int> &stream, int N) {
  for (int i = 0; i < N; ++i) {
    #pragma HLS PIPELINE II=1
    stream.Push(memory[i]);
  }
}

void Write(Stream<int> &stream, int *memory, int N) {
  for (int i = 0; i < N; ++i) {
    #pragma HLS PIPELINE II=1
    memory[i] = stream.Pop();
  }
}

void MyKernel(int const *source_memory, int *destination_memory, int N) {
  #pragma HLS INTERFACE m_axi port=source_memory bundle=gmem0
  #pragma HLS INTERFACE m_axi port=destination_memory bundle=gmem1
  #pragma HLS INTERFACE s_axilite port=source_memory bundle=control
  #pragma HLS INTERFACE s_axilite port=destination_memory bundle=control
  #pragma HLS INTERFACE s_axilite port=N bundle=control
  Stream<int> stream(512);
  Read(source_memory, stream, N);
  Write(stream, destination_memory, N);
}

 

Tags (3)
1 Solution

Accepted Solutions
Highlighted
Xilinx Employee
Xilinx Employee
487 Views
Registered: ‎08-22-2019

Hi,

Totally agree with the points you made. I did not mean to imply that the pragma option was going away. This issue is only for the case when the stream is a class member variable. Further discussion with R&D resulted in the following modified design that allows you to get back to what you originally had:

Note that you need to inline the Stream constructor for the pragma to work correctly. So in summary, both forms of hls::stream will exist to allow backward compatibility. The only constraint is that Vitis HLS expects a constant for the depth option in the pragma and not an argument. An error will be emitted for this case. 

#include <hls_stream.h>

template<typename T, int U=0>
class Stream;

template <typename T>
class Stream<T, 0> {

public:
Stream() {}

T Pop() {
#pragma HLS INLINE
return stream_.read();
}

void Push(T const &val) {
#pragma HLS INLINE
stream_.write(val);
}

protected:
hls::stream<T> stream_;
};

template<typename T, int U>
class Stream : public Stream<T, 0> {
public:
Stream() {
#pragma HLS inline
#pragma HLS STREAM variable=this->stream_ depth=U
}
};

void Read(int const *memory, Stream<int> &stream, int N) {
for (int i = 0; i < N; ++i) {
#pragma HLS PIPELINE II=1
stream.Push(memory[i]);
}
}

void Write(Stream<int> &stream, int *memory, int N) {
for (int i = 0; i < N; ++i) {
#pragma HLS PIPELINE II=1
memory[i] = stream.Pop();
}
}

void MyKernel(int const *source_memory, int *destination_memory, int N) {
#pragma HLS INTERFACE m_axi bundle=gmem0 port=source_memory
#pragma HLS INTERFACE m_axi port=destination_memory bundle=gmem1
#pragma HLS INTERFACE s_axilite port=source_memory bundle=control
#pragma HLS INTERFACE s_axilite port=destination_memory bundle=control
#pragma HLS INTERFACE s_axilite port=N bundle=control
Stream<int,512> stream;
Read(source_memory, stream, N);
Write(stream, destination_memory, N);
}

View solution in original post

10 Replies
Highlighted
Xilinx Employee
Xilinx Employee
537 Views
Registered: ‎08-22-2019

Hi,

Thanks for asking this question and providing the code example. Yes, the behavior of hls::stream has changed in Vitis HLS as compared to Vivado HLS. Here are the main differences:

  • hls::stream<typename T, int depth> - In Vitis HLS, the depth of the stream can now be specified via an additional template parameter (instead of the old style definition via a pragma option)
  • Additionally, pragmas on class member variables (like stream_ in your Stream class) need to be specified in the class constructor with the caveat that any pragma settings must be statically determinable. So the way you set the depth of the stream (via a constructor argument) is no longer possible/supported. The tool will error out for such a use case in the future. 
  • Finally, a Vitis HLS migration guide is available at https://www.xilinx.com/support/documentation/sw_manuals/xilinx2020_1/ug1391-vitis-hls-migration-guide.pdf

Given the above conditions/limitations, I edited your code example to be as follows:

 #include <hls_stream.h>
#define W 512

template <typename T, int U>
class Stream {

public:

Stream() {
#pragma HLS INLINE
#pragma HLS STREAM variable=stream_
}

T Pop() {
#pragma HLS INLINE
return stream_.read();
}

void Push(T const &val) {
#pragma HLS INLINE
stream_.write(val);
}

private:

hls::stream<T, U> stream_;
};

void Read(int const *memory, Stream<int, W> &stream, int N) {
for (int i = 0; i < N; ++i) {
#pragma HLS PIPELINE II=1
stream.Push(memory[i]);
}
}

void Write(Stream<int, W> &stream, int *memory, int N) {
for (int i = 0; i < N; ++i) {
#pragma HLS PIPELINE II=1
memory[i] = stream.Pop();
}
}

void MyKernel(int const *source_memory, int *destination_memory, int N) {
#pragma HLS INTERFACE m_axi port=source_memory bundle=gmem0
#pragma HLS INTERFACE m_axi port=destination_memory bundle=gmem1
#pragma HLS INTERFACE s_axilite port=source_memory bundle=control
#pragma HLS INTERFACE s_axilite port=destination_memory bundle=control
#pragma HLS INTERFACE s_axilite port=N bundle=control
Stream<int, W> stream;
Read(source_memory, stream, N);
Write(stream, destination_memory, N);
}

 

Hope this helps. 

0 Kudos
Highlighted
Adventurer
Adventurer
508 Views
Registered: ‎02-16-2017

Hi Raman,

Thanks for the detailed response.

The template-based interface to setting stream depth is a nice addition, and I will update hlslib to use this.

Have you considered maintaining support for the old syntax? Here's a few arguments why I think this is a good idea:

  • The template-based syntax requires the depth to be reflected in function arguments. This means that any function that accepts streams as arguments must be specialized to every callsite to match the stream depths of the callsite. This is unproductive for writing libraries and modular code. For example, if I have a module "MyBinaryOp" that consumes 2 input streams and produces 1 output stream (this is a common scenario), it now needs to have 3 additional template arguments to allow passing streams of any depth.
  • Adding or removing the depth arguments means changing it in 5 different places (stream declaration, producer declaration, producer definition, consumer declaration, producer definition), not to mention adding or removing template arguments.
  • Tying the stream depth to the producer and consumer definitions seems conceptually unintuitive, since the reason for setting stream depths is usually not due to each individual module, but due to how they are interconnected in the top module.
  • Backwards compatibility: existing codes break because of the removal of this feature. I have to make a large number of tedious changes to all my codes now (in particular, I have to add a lot of template arguments) if I want to support newer versions of Vitis. Codes that are infrequently maintained can simply die because of changes like this, which is a shame.
  • Compatibility between vivado_hls and vitis_hls: it seems reasonable to expect that the same HLS code will be compatible with either frontend.



Maybe this change was not intentional (since no error is emitted when using non-constexpr values) - something to do with the constant propagation pass that has to happen before pragmas are interpreted.

0 Kudos
Highlighted
Xilinx Employee
Xilinx Employee
488 Views
Registered: ‎08-22-2019

Hi,

Totally agree with the points you made. I did not mean to imply that the pragma option was going away. This issue is only for the case when the stream is a class member variable. Further discussion with R&D resulted in the following modified design that allows you to get back to what you originally had:

Note that you need to inline the Stream constructor for the pragma to work correctly. So in summary, both forms of hls::stream will exist to allow backward compatibility. The only constraint is that Vitis HLS expects a constant for the depth option in the pragma and not an argument. An error will be emitted for this case. 

#include <hls_stream.h>

template<typename T, int U=0>
class Stream;

template <typename T>
class Stream<T, 0> {

public:
Stream() {}

T Pop() {
#pragma HLS INLINE
return stream_.read();
}

void Push(T const &val) {
#pragma HLS INLINE
stream_.write(val);
}

protected:
hls::stream<T> stream_;
};

template<typename T, int U>
class Stream : public Stream<T, 0> {
public:
Stream() {
#pragma HLS inline
#pragma HLS STREAM variable=this->stream_ depth=U
}
};

void Read(int const *memory, Stream<int> &stream, int N) {
for (int i = 0; i < N; ++i) {
#pragma HLS PIPELINE II=1
stream.Push(memory[i]);
}
}

void Write(Stream<int> &stream, int *memory, int N) {
for (int i = 0; i < N; ++i) {
#pragma HLS PIPELINE II=1
memory[i] = stream.Pop();
}
}

void MyKernel(int const *source_memory, int *destination_memory, int N) {
#pragma HLS INTERFACE m_axi bundle=gmem0 port=source_memory
#pragma HLS INTERFACE m_axi port=destination_memory bundle=gmem1
#pragma HLS INTERFACE s_axilite port=source_memory bundle=control
#pragma HLS INTERFACE s_axilite port=destination_memory bundle=control
#pragma HLS INTERFACE s_axilite port=N bundle=control
Stream<int,512> stream;
Read(source_memory, stream, N);
Write(stream, destination_memory, N);
}

View solution in original post

Highlighted
Adventurer
Adventurer
455 Views
Registered: ‎02-16-2017

Thank you for the additional suggestion. I already implemented the template-argument based version for hlslib::Stream (unfortunately there's an additional issue with the new hls::stream).

However, this does not give me back what I had: the issue remains that the modified Stream class that uses hls::stream as a member variable only works via template argument, which gives rise to all the issues I raised when using hlslib::Stream or any object with an hls::stream member variable in function arguments (needing one template argument per stream argument to a function, having to make changes in 5 places, compatibility, etc.)

Of course you are not committed to maintaining support for third party libraries, but I think it's a shame to lose the capability to extend hls::stream in this way.

Is there a fundamental reason why you cannot do constant propagation before evaluating pragmas, like you did before?

0 Kudos
Highlighted
Xilinx Employee
Xilinx Employee
442 Views
Registered: ‎08-22-2019

I am not sure I follow. If you look at the modified example I sent, the derived class approach allows you to refer to the stream without the depth (using the base class pointer) and so your interfaces for Read and Write functions dont need to change. 

Also the issue with naming the stream is a bug that that has been fixed in the upcoming 2020.2 release. 

0 Kudos
Highlighted
Adventurer
Adventurer
429 Views
Registered: ‎02-16-2017

Apologies, I hadn't understood the purpose of the code you linked. I'm playing with it now, and indeed this seems to work for the small example. I have never seen this technique of inheriting from another specialization of the same class, seems a little dirty...

With a more complex code, I'm getting an internal LLVM error:

Error in llvm-link
    while executing
"source Synthesis.tcl"
    ("uplevel" body line 1)
    invoked from within
"uplevel \#0 [list source $arg] "

However, I need some time to narrow down what causes this error. Thanks for the advice so far!

0 Kudos
Highlighted
Adventurer
Adventurer
371 Views
Registered: ‎02-16-2017

@ramananrYour suggestion works for vitis_hls 2020.1. However, this solution is not compatible with vivado_hls 2020.1: it does not seem to support the "this->" syntax:

error: cannot pass object of non-trivial type 'hls::stream<int>' through variadic function; call will abort at runtime [-Wnon-pod-varargs
]                            
_ssdm_SpecStream( this->stream_, 0, depth, "");                                                                                              
                        ^

So even if we don't consider older versions of either toolflow, the two tools unfortunately seem to have diverged.

The only way I can think of to make my library compatible with any tool or version is use #ifdef's to inline different code depending on which tool is being used. However, I have not found a portable way to do this - perhaps you can help?

0 Kudos
Highlighted
Xilinx Employee
Xilinx Employee
331 Views
Registered: ‎08-22-2019

Hi,

Vitis HLS 2020.2 will contain the predefined macro "__VITIS_HLS__" and so users can use "#ifdef __VITIS_HLS__" to ensure that the code block contained in the ifdef gets executed only when running Vitis HLS. 

But unfortunately, previous versions of Vitis HLS and Vivado HLS do not contain such macros and therefore, if you wish to use such macros in your library, you will have to instruct your users to add "-D__VITIS_HLS__" in the -cflags and -csimflags arguments of the add_files command.

Highlighted
Adventurer
Adventurer
267 Views
Registered: ‎02-16-2017
Thanks Raman, this will be very helpful! Have you considered also providing the current version of the compiler like gcc and clang do (__GNUC__, __GNUC_MINOR__, __clang_major__, __clang_minor__)?
0 Kudos
Highlighted
Xilinx Employee
Xilinx Employee
254 Views
Registered: ‎08-22-2019

Yes, this is under consideration and being discussed in R&D.