circt icon indicating copy to clipboard operation
circt copied to clipboard

CFToHandshake header depends on Transforms but doesn't actually ensure the dependency is built

Open mikeurbach opened this issue 1 year ago • 0 comments

CFToHandshake is a Conversion, but it depends on Transforms: https://github.com/llvm/circt/blob/b94d4b1d99e44bf7537a8c6b22c81395d5c527bf/include/circt/Conversion/CFToHandshake.h#L20

Specifically, it uses maximizeSSA here: https://github.com/llvm/circt/blob/b94d4b1d99e44bf7537a8c6b22c81395d5c527bf/include/circt/Conversion/CFToHandshake.h#L73

This isn't great for layering, because anything that includes Conversion/Passes.h is including this, but some of these other conversions might not build Transforms at all. For example, this popped up a couple times while building Python wheels, where a C API that included Conversion/Passes.h failed because Transforms/Passes.h.inc was never generated.

https://github.com/llvm/circt/actions/runs/7871397080/job/21474609696#step:6:3092 https://github.com/llvm/circt/actions/runs/7474242994/job/20339994666#step:6:3164

I would normally fix this layering by moving the function in question to the implementation file, move the dependency on Transforms/Passes.h into the implementation file, and add a DEPENDS in the library's CMakeLists.txt on CIRCTransformPassIncGen. In fact, I did that, but I realized the function in question has some template arguments and is trying to be used in other places, which doesn't work.

@mortbopet can you take a look? I don't know what this function does any more, but it seems like it was factored this way to share with Ibis.

mikeurbach avatar Feb 14 '24 06:02 mikeurbach