[CIR][NFC] move data member pointer lowering to CXXABI #1130
Conversation
| mlir::Value loweredAddr, mlir::Value loweredMember, | ||
| mlir::OpBuilder &builder) const { | ||
| auto llvmElementTy = mlir::IntegerType::get(op.getContext(), 8); | ||
| return builder.create<mlir::LLVM::GEPOp>( |
There was a problem hiding this comment.
We should not assume LLVM lowering at this level and emit a GEP here, target lowering is supposed to understand CIR to CIR. In theory CIR types and etc should be already on the right ABI, so that LLVM lowering has no extra work. Have you tried only returning the necessary information for lower to LLVM to emit the GEP?
There was a problem hiding this comment.
In theory CIR types and etc should be already on the right ABI, so that LLVM lowering has no extra work.
Basically this PR does ABI lowering work. Thus I'm thinking that I should move the changes here to some prior pass that do ABI lowering works. LoweringPrepare seems good to me at first but I found it a bit difficult to lower type, attributes, and ops all at the same time there, and I'm not sure whether it's conceptually right to do ABI lowering during LoweringPrepare. Should we invent a new ABI lowering pass for this kind of stuff? CallConvLowering already does some ABI related lowering but that's for function calls and function signatures only.
There was a problem hiding this comment.
It's still fine if these things return CIR-only operations to LowerToLLVM, my understanding is that those will get scheduled to be lowered when the pass continues. Did you try that? Example: when lowering casts we call the rewriter to emit cir::CmpOp, etc.
There was a problem hiding this comment.
Did you try that? Example: when lowering casts we call the rewriter to emit cir::CmpOp, etc.
Wow didn't try this. This is indeed a good way to resolve this if it works. Will try later.
| // the class object containing it, represented as a ptrdiff_t | ||
| const clang::TargetInfo &target = LM.getTarget(); | ||
| unsigned width = | ||
| target.getTypeWidth(target.getPtrDiffType(clang::LangAS::Default)); |
There was a problem hiding this comment.
sounds like this method should be about returning width instead? Answering LLVM type questions at this point sounds like layer violation?
Lancern
force-pushed
the
data-member-lowering
branch
2 times, most recently
from
a564ce0
to
02e6c13
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Lancern
force-pushed
the
data-member-lowering
branch
from
02e6c13
to
cc18fbf
Compare
|
Fixed format issues and rebased. |
This patch moves the lowering code for data member pointers from the conversion patterns to the implementation of CXXABI because this part should be ABI- specific.
Lancern
force-pushed
the
data-member-lowering
branch
from
cc18fbf
to
9529507
Compare
This PR moves the lowering code for data member pointers from the conversion patterns to the implementation of CXXABI because this part should be ABI-specific.