[CIR][CIRGen] Enable address space flags and re-categorize / fix usages #614
Conversation
| @@ -128,7 +128,6 @@ struct CIRGenTypeCache { | |||
| // Address spaces are not yet fully supported, but the usage of the default | |||
| // alloca address space can be used for now only for comparison with the | |||
| // default address space. | |||
| assert(!UnimplementedFeature::addressSpace()); | |||
There was a problem hiding this comment.
I believe you should switch this with addressSpaceCasting :)
| LangAS AAS = getASTAllocaAddressSpace(); | ||
| auto TargetAAS = CGM.getASTContext().getTargetAddressSpace(AAS); |
There was a problem hiding this comment.
Clang's code sets the alloca's address space to the one described by the data-layout, and here you are using another path in the codegen - the targetCIRGenInfo/targetCodeGenInfo path. Not sure what's the right choice here, need to dive into a rabbit hole to understand the original intention of use for both.
In general, we are trying to stick as much as possible to clang's codegen structure to avoid re-making past mistakes :D
I'll note that it do seem like I missed this details in the alloca builder I implemented a while ago! I guess we should either understand how to pass the datalayout here or add some nice TODO comment for future us :)
There was a problem hiding this comment.
For the two methods to get the alloca addrspace, I think these lines of comment helps. I noticed that the code is creating alloca and cast it immediately, therefore I naively simplify it to a direct alloca😂. Sorry for the mistake.
clangir/clang/lib/CodeGen/CGBuiltin.cpp
Lines 4082 to 4088 in 3da10fa
Most of targets in current codebase indeed implement getASTAllocaAddressSpace with DataLayout internally. The intention here should be to ensure the generality. (I don't fully understand the case of automatic variables mentioned in the comment above.)
Anyway, shall we nicely have a addressSpaceCasting() mark here? For reference DataLayout directly, I know CIR uses the MLIR model, which has getAllocaMemorySpace method. It's not public through CIRDataLayout though, probably skipped at that time. This should be a workable path.😉
| @@ -1206,23 +1206,21 @@ RValue CIRGenFunction::buildCall(clang::QualType CalleeType, | |||
| // some trivial cases]. | |||
| // That is, in the general case, we should assume that a call through an | |||
| // unprototyped function type works like a *non-variadic* call. The way we | |||
| // make this work is to cast to the exxact type fo the promoted arguments. | |||
| // make this work is to cast to the exact type fo the promoted arguments. | |||
There was a problem hiding this comment.
Nittiest of picks - if you are already here:
| // make this work is to cast to the exact type fo the promoted arguments. | |
| // make this work is to cast to the exact type of the promoted arguments. |
| assert(!UnimplementedFeature::addressSpaceInGlobalVar()); | ||
| Addr = builder.create<mlir::cir::GetGlobalOp>( |
There was a problem hiding this comment.
Do you think it would make sense to add an assert here that the address space is the default (0)? Future users may try to understand why some verifier fail downstream from the codegen, and it would be hard to trace it back to this very low-level path in the code :)
| auto CalleePtrTy = mlir::cir::PointerType::get(builder.getContext(), | ||
| CalleeTy, CalleePtrAS); |
There was a problem hiding this comment.
Do you think you can hide the mlir::cir::PointerType::get(builder.getContext(), part in a CIRGenBuilder (or CIRBaseBuilder) method with a name along getPointerTy?
Can you use getPointerTo (from CIRBaseBuilder) instead? Hides some of the horrors ;)
|
Many thanks for reviewing this mess😂! I also found a bunch of extra usages to mark. I shall go through entirely. For the test cases, I would better split this PR into two parts, one for NFC flags, another for fix with tests, making it tidier. |
Depend on #606 , will rebase after its merging.
This PR fixes simple cases and re-categorize other cases to
addressSpaceInGlobalVaroraddressSpaceCasting. Maybe the changes are hard to review😂. But I will solve them in months, so no need to be strict with the precise categorization.