calyx icon indicating copy to clipboard operation
calyx copied to clipboard

Remove `timeout` argument from Xilinx backend

Open zzy666666zzy opened this issue 2 years ago • 7 comments
trafficstars

Hi @rachitnigam ,

After solving #1470 and #1575, we generated .xclbin from our gemm kernel referring to Calyx's tutorial.

https://docs.calyxir.org/fud/xilinx.html

I am invoking the kernel in an OpenCL host code on Petalinux platform. Afterward, I refer to this reference design

https://github.com/Xilinx/Vitis_Accel_Examples/tree/master/rtl_kernels/rtl_vadd

to cross-compile the host OpenCL code and export the host executable and xclbin to the Petalinux. But I got errors from clSetKernelArg() saying that

ERROR: clSetKernelArg() for kernel "Toplevel", argument index 0.
Error calling err = krnl_gemm.setArg(0, buffer_w), error code is: -51

I wonder is there any reference design from Calyx about how to write host code and how to invoke the kernel using XRT or OpenCL? I attached my necessary files in case you have any pertinent solutions to it. Thanks a lot!

kernel_xml.txt host_cpp.txt

zzy666666zzy avatar Jul 23 '23 21:07 zzy666666zzy

Problem solved.

The reason is that I didn't set the timeout argument with index 0. I randomly assign a number, say 10, to timeout argument in my host OpenCL code and got the correct result.

I am wondering is timeout is an important parameter or not. Because it is not associated with essential logics in generated Toplevel verilog code.

zzy666666zzy avatar Jul 24 '23 12:07 zzy666666zzy

Yeah, looks like our harness for running things on the FPGA also does not use the timeout argument but the kernel.xml file is generated to expect it? Not sure if Vivado requires it specifically or not. @sampsyo any clue?

rachitnigam avatar Jul 24 '23 20:07 rachitnigam

I have actually always been a little confused about what timeout was supposed to do. Is it a cycle limit for the accelerator invocation, so "runaway" Calyx programs nonetheless complete the XRT transaction? That seems most likely to me, and perhaps it was just never implemented!

No pressure at all, but if @sgpthomas has a recollection of what this was for, we would be grateful!

sampsyo avatar Jul 25 '23 01:07 sampsyo

It was a long time ago, but I think you're right @sampsyo. I think I used it during testing to handle runaway programs. I assume that I just never published my use of the timeout parameter, but accidentally left it in the code. I think it's probably safe to remove if it's no longer needed.

sgpthomas avatar Jul 25 '23 20:07 sgpthomas

Thanks for confirming @sgpthomas! @zzy666666zzy we'll try to remove this extra argument in an upcoming PR.

rachitnigam avatar Jul 25 '23 21:07 rachitnigam

Indeed; many thanks, @sgpthomas! That makes lots of sense to me. Seems like it wouldn't exactly hurt to have such a timeout, but given that this stuff is basically working these days (i.e., indefinite hangs are not the main problem we're facing with AXI/Xilinx stuff), we can shed it instead of implementing it.

sampsyo avatar Jul 25 '23 23:07 sampsyo

It can in fact be an option in our AXI generator from https://github.com/cucapra/calyx/issues/1603

rachitnigam avatar Jul 25 '23 23:07 rachitnigam