llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
307 stars 84 forks source link

[CIR][ThroughMLIR] Fix FuncOp for functions with pointer arguments. #684

Closed Kritoooo closed 2 weeks ago

Kritoooo commented 3 weeks ago

This PR is to fix the issue #658 . Now we can get the correct result using the following command.

echo "void test(int *){}" |  ./build/Debug/bin/clang -cc1 -triple x86_64-unknown-linux-gnu -fclangir -fno-clangir-direct-lowering -emit-mlir -o -

result:

module attributes {cir.lang = #cir.lang<c>, cir.sob = #cir.signed_overflow_behavior<undefined>, cir.triple = "x86_64-unknown-linux-gnu", dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<!llvm.ptr, dense<64> : vector<4xi64>>, #dlti.dl_entry<i1, dense<8> : vector<2xi64>>, #dlti.dl_entry<f16, dense<16> : vector<2xi64>>, #dlti.dl_entry<i32, dense<32> : vector<2xi64>>, #dlti.dl_entry<i8, dense<8> : vector<2xi64>>, #dlti.dl_entry<i16, dense<16> : vector<2xi64>>, #dlti.dl_entry<!llvm.ptr<272>, dense<64> : vector<4xi64>>, #dlti.dl_entry<!llvm.ptr<271>, dense<32> : vector<4xi64>>, #dlti.dl_entry<!llvm.ptr<270>, dense<32> : vector<4xi64>>, #dlti.dl_entry<f128, dense<128> : vector<2xi64>>, #dlti.dl_entry<f64, dense<64> : vector<2xi64>>, #dlti.dl_entry<f80, dense<128> : vector<2xi64>>, #dlti.dl_entry<i128, dense<128> : vector<2xi64>>, #dlti.dl_entry<i64, dense<64> : vector<2xi64>>, #dlti.dl_entry<"dlti.stack_alignment", 128 : i64>, #dlti.dl_entry<"dlti.endianness", "little">>, llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"} {
  func.func @test(%arg0: memref<i32> loc(fused[#loc3, #loc4])) {
    %alloca = memref.alloca() {alignment = 8 : i64} : memref<memref<i32>> loc(#loc7)
    memref.store %arg0, %alloca[] : memref<memref<i32>> loc(#loc5)
    return loc(#loc2)
  } loc(#loc6)
} loc(#loc)

And the test/CIR/Lowering/ThroughMLIR/dot.cir now passes the test, so I have removed the XFAIL flag.

Lancern commented 2 weeks ago

Do we need to consider ABI problems here? @SchrodingerZhu pointed out that memref type is not lowered to a pointer; instead it is a fat pointer that also captures various additional information such as the shape, see the comments for LLVMTypeConverter::getMemRefDescriptorFields:

https://github.com/llvm/clangir/blob/0c86870814df33e7895248f2341c046105dac475/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp#L311-L326

If ABI should be concerned we may revert this PR.

Kritoooo commented 2 weeks ago

Do you mean that we should not simply convert cir.ptr to memref? If such a conversion has issues, you can revert this PR at any time.

Kritoooo commented 2 weeks ago

However, I need to clarify that this PR only addresses the issue where cir.ptr or cir.vec (and similar types) cannot be correctly converted when used as function parameters. In the original lowering of FuncOp, due to the order of statements, the parameters carried by the entry block were converted multiple times (I am not exactly sure why; I pinpointed this statement using the debugger). Therefore, an error occurs in the final convertRegionTypes where the conversion cannot be completed. Regarding ABI, I'm not very familiar with it, so I can't provide useful assistance on that matter.

Lancern commented 2 weeks ago

Do you mean that we should not simply convert cir.ptr to memref?

The semantics are equivalent, I'm just not sure if it's OK to break C/C++ ABI guarantees on the MLIR lowering path. If ABI is not the problem this lowering is good.