[CIR][ABI][NFC] Add CC lowering for void CallOps #668

sitio-couto · 2024-06-07T10:22:27Z

This patch implements the lowering of function calls that receive and return void. In practice, nothing has to be done (at least for the x86 ABI), so this case is used as a primer for the target lowering library since it helps populate the base logic for handling calling convention lowering of function calls.

This patch implements the lowering of function calls that receive and return void. In practice, nothing has to be done (at least for the x86 ABI), so this case is used as a primer for the target lowering library since it helps populate the base logic for handling calling convention lowering of function calls without adding too much complexity.

bcardosolopes

Some nits but overall good, thanks!

Please try not to duplicate things if they already exist in CIR, take the opportunity to unify/refactor them, it will make landing your PRs faster - most of the comments are very similar to the ones I added in the previous PR.

Btw, I thought we had an agreement about the next PR being about merging the existing ABI lowering logic to the stuff you're adding, can you please clarify the plans here?

clang/include/clang/CIR/MissingFeatures.h

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerCall.h

clang/lib/CIR/Dialect/Transforms/TargetLowering/Targets/X86.cpp

sitio-couto · 2024-06-07T23:41:43Z

@bcardosolopes applied and answered.

I thought we had an agreement about the next PR being about merging the existing ABI

I understood that we would just move all ABI stuff to the same for now and unify them later. Regardless, it would be better to prime the library first and unify the ABI logic later to simplify the process.

please clarify the plans here?

My plan was to leave it as is until I have reached structs/pointers in the calling convention lowering pass. After that, I would start unifying the ABI logic updating stuff like VAArg lowering to follow codegen parity.

bcardosolopes · 2024-06-10T18:12:53Z

My plan was to leave it as is until I have reached structs/pointers in the calling convention lowering pass. After that, I would start unifying the ABI logic updating stuff like VAArg lowering to follow codegen parity.

My problem with that is if another PR shows up that needs to touch that part, we're gonna be creating more techinical debt. In that case we're gonna have to block those on top of your work, and I'd appreciate then if you are the one during the review and making sure the skeleton is appropriate, does that work for you?

bcardosolopes

code formatting needs update

clang/include/clang/CIR/MissingFeatures.h

clang/include/clang/CIR/x86.h

clang/test/CIR/Transforms/Target/x86/x86-call-conv-lowering-pass.cpp

sitio-couto · 2024-06-11T00:02:04Z

@bcardosolopes applied.

bcardosolopes

LGTM

This reverts commit 901f532. Revert as a prereq to revert #668 This broke the build: ``` /usr/bin/ld: lib/libMLIRCIRTransforms.a(LoweringPrepare.cpp.o): in function `(anonymous namespace)::LoweringPreparePass::lowerVAArgOp(mlir::cir::VAArgOp)': /local/home/dolsen/clangir/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp:340: undefined reference to `cir::CIRDataLayout::CIRDataLayout(mlir::ModuleOp)' /usr/bin/ld: lib/libTargetLowering.a(LowerTypes.cpp.o): in function `cir::CIRDataLayout::CIRDataLayout(llvm::StringRef, mlir::ModuleOp)': /local/home/dolsen/clangir/clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h:31: undefined reference to `cir::CIRDataLayout::reset(llvm::StringRef)'`' ``` Steps to reproduce: ``` $ cmake -GNinja -DCMAKE_INSTALL_PREFIX=/<some-path>/clangir-install "-DLLVM_ENABLE_PROJECTS=clang;mlir" -DCLANG_ENABLE_CIR=ON -DLLVM_TARGETS_TO_BUILD=host -DCMAKE_BUILD_TYPE=Debug ../clangir/llvm $ ninja install ```

This reverts commit 66bb15b. This broke the build: ``` /usr/bin/ld: lib/libMLIRCIRTransforms.a(LoweringPrepare.cpp.o): in function `(anonymous namespace)::LoweringPreparePass::lowerVAArgOp(mlir::cir::VAArgOp)': /local/home/dolsen/clangir/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp:340: undefined reference to `cir::CIRDataLayout::CIRDataLayout(mlir::ModuleOp)' /usr/bin/ld: lib/libTargetLowering.a(LowerTypes.cpp.o): in function `cir::CIRDataLayout::CIRDataLayout(llvm::StringRef, mlir::ModuleOp)': /local/home/dolsen/clangir/clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h:31: undefined reference to `cir::CIRDataLayout::reset(llvm::StringRef)'`' ``` Steps to reproduce: ``` $ cmake -GNinja -DCMAKE_INSTALL_PREFIX=/<some-path>/clangir-install "-DLLVM_ENABLE_PROJECTS=clang;mlir" -DCLANG_ENABLE_CIR=ON -DLLVM_TARGETS_TO_BUILD=host -DCMAKE_BUILD_TYPE=Debug ../clangir/llvm $ ninja install ```

This patch implements the lowering of function calls that receive and return void. In practice, nothing has to be done (at least for the x86 ABI), so this case is used as a primer for the target lowering library since it helps populate the base logic for handling calling convention lowering of function calls.

Essentially re-applies #668 and #678, but also includes #687 which patched build introduced by the other two PRs. Closes #691

This patch implements the lowering of function calls that receive and return void. In practice, nothing has to be done (at least for the x86 ABI), so this case is used as a primer for the target lowering library since it helps populate the base logic for handling calling convention lowering of function calls.

This reverts commit 901f532. Revert as a prereq to revert llvm#668 This broke the build: ``` /usr/bin/ld: lib/libMLIRCIRTransforms.a(LoweringPrepare.cpp.o): in function `(anonymous namespace)::LoweringPreparePass::lowerVAArgOp(mlir::cir::VAArgOp)': /local/home/dolsen/clangir/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp:340: undefined reference to `cir::CIRDataLayout::CIRDataLayout(mlir::ModuleOp)' /usr/bin/ld: lib/libTargetLowering.a(LowerTypes.cpp.o): in function `cir::CIRDataLayout::CIRDataLayout(llvm::StringRef, mlir::ModuleOp)': /local/home/dolsen/clangir/clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h:31: undefined reference to `cir::CIRDataLayout::reset(llvm::StringRef)'`' ``` Steps to reproduce: ``` $ cmake -GNinja -DCMAKE_INSTALL_PREFIX=/<some-path>/clangir-install "-DLLVM_ENABLE_PROJECTS=clang;mlir" -DCLANG_ENABLE_CIR=ON -DLLVM_TARGETS_TO_BUILD=host -DCMAKE_BUILD_TYPE=Debug ../clangir/llvm $ ninja install ```

This reverts commit 66bb15b. This broke the build: ``` /usr/bin/ld: lib/libMLIRCIRTransforms.a(LoweringPrepare.cpp.o): in function `(anonymous namespace)::LoweringPreparePass::lowerVAArgOp(mlir::cir::VAArgOp)': /local/home/dolsen/clangir/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp:340: undefined reference to `cir::CIRDataLayout::CIRDataLayout(mlir::ModuleOp)' /usr/bin/ld: lib/libTargetLowering.a(LowerTypes.cpp.o): in function `cir::CIRDataLayout::CIRDataLayout(llvm::StringRef, mlir::ModuleOp)': /local/home/dolsen/clangir/clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h:31: undefined reference to `cir::CIRDataLayout::reset(llvm::StringRef)'`' ``` Steps to reproduce: ``` $ cmake -GNinja -DCMAKE_INSTALL_PREFIX=/<some-path>/clangir-install "-DLLVM_ENABLE_PROJECTS=clang;mlir" -DCLANG_ENABLE_CIR=ON -DLLVM_TARGETS_TO_BUILD=host -DCMAKE_BUILD_TYPE=Debug ../clangir/llvm $ ninja install ```

Essentially re-applies llvm#668 and llvm#678, but also includes llvm#687 which patched build introduced by the other two PRs. Closes llvm#691