-
Notifications
You must be signed in to change notification settings - Fork 53
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
Added --disable-init flag to fud xclbin stage #1085
Conversation
LGTM |
Seems good if it works! Just thinking through the implications here for a sec… this pass disables all our "automatic initialization" stuff when generating Verilog but only for Xilinx execution. I hope that none of the code is relying on any kind of initialization… @andrewb1999's original advice in #1071 suggests that it was not necessary to get things working:
But I wonder if this doesn't actually indicate a deeper problem. Namely, perhaps the initialization we're doing is wrong not just in a Xilinx context but for all Verilog generation—and we just need to back off on some of that altogether. It would take a little more digging into how we currently initialize things to know for sure, but perhaps there's a subtler change to be made here. I would still recommend (1) forging ahead with this PR as-is, if it actually seems to improve things for Xilinx execution, and then (2) revisiting this broader "is our initialization stuff correct?" question later, with a goal toward removing the special |
Yeah to be clear I think this is an issue with the verilog initialization in all cases, not just Xilinx. I think this is a case where different simulators/compilers will behave differently on something akin to undefined behavior in the systemverilog spec. A good example of this behavior in the Other simulators may handle this situation differently, but it is certainly the case that if we remove the initialization of That being said, I agree with @sampsyo that this PR is a good starting place and we can come back to fixing the core issue with initialization later. |
This disables all automatic initialization when generating verilog, but only for Xilinx execution. At some point thid may be revisited to a broader "is initilization correct." Previously, behavior was wonky because we were treating certain signals as both registers (in initilization) and wires (later on).
* Changed axi manager port width to 512 * Added comments regarding kernel width Co-authored-by: Nathaniel Navarro <[email protected]>
94224d9
to
cbbb2c5
Compare
Fixes #1080, which is part of #1072