MaskRay wrote: This PR should not add llvm/lib/Support/AMDGPUPrintf.cpp to llvm/lib/Support/.
``` The cycle argument The author's reply to koparasy says Frontend/Offloading would create a cycle. That is true but beside the point: LLVMFrontendOffloading already depends on TransformUtils. The obvious home was TransformUtils, which is what the first revision of the PR used: - No link cost. clangCIRLoweringDirectToLLVM already pulls in TransformUtils through MLIRLLVMToLLVMIRTranslation → MLIRTargetLLVMIRExport, whose LINK_COMPONENTS include TransformUtils. Clang links it via clangCodeGen anyway. Adding TransformUtils to the CIR CMakeLists only states a dependency that already exists. - koparasy's actual concern was small. It was about pulling in IRBuilder, i.e. AMDGPUEmitPrintf.h including llvm/IR/IRBuilder.h. That costs nothing next to the MLIR headers that TU already includes. If it matters, a small header in llvm/Transforms/Utils/ without IRBuilder fixes it. koparasy also wrote "No strong feelings about this one." Suggested follow-up: - Move the declarations into llvm/Transforms/Utils/AMDGPUEmitPrintf.h and the definitions back into AMDGPUEmitPrintf.cpp. - Add TransformUtils back to clang/lib/CIR/Lowering/DirectToLLVM/CMakeLists.txt. - Remove AMDGPUPrintf.cpp from llvm/lib/Support/CMakeLists.txt and undo the gn port. ``` https://github.com/llvm/llvm-project/pull/231563 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
