Calling Convention Lowering: support function pointers

[gitoleg]

Hi all.
I faced with the necessity of function pointer support in the calling convention lowering pass. And here are several examples that demonstrate the complexity of the problem. I don't even touch calls to functions at all - it's not a problem so far.

The next types are used in the examples below:

typedef struct {
  int a;
} S; 

typedef int (*myfptr)(S); 

typedef struct {
  myfptr f;
} T; 

Example 1

int foo(S s) { return s.a; }

void bar() {
    S s;
    myfptr f = foo;
    f(s);
}

%1 = cir.alloca !cir.ptr<!cir.func<!s32i (!ty_S)>>, !cir.ptr<!cir.ptr<!cir.func<!s32i (!ty_S)>>>, ["a", init] {alignment = 8 : i64}
%2 = cir.get_global @foo : !cir.ptr<!cir.func<!s32i (!ty_S)>>
cir.store %2, %1 : !cir.ptr<!cir.func<!s32i (!ty_S)>>, !cir.ptr<!cir.ptr<!cir.func<!s32i (!ty_S)>>> 

As one can see, the code above needed to be rewritten in order to comply with a calling convetion.

First of all, this code won't compile at all:

'cir.get_global' op result type pointee type ''!cir.func<!cir.int<s, 32> (!cir.struct<struct "S" {!cir.int<s, 32>} #cir.record.decl.ast>)>'' does not match type '!cir.func<!cir.int<s, 32> (!cir.int<s, 32>)>' of the global @foo

Since @foo has the proper type already: '!cir.func<!cir.int<s, 32> (!cir.int<s, 32>)>' Ok. we can fix this - and rewrite GetGlobalOp.
But next we have to do something with AllocaOp - we can either perform a cast of the get_global result or rewrite the alloca operation. The former is easier but is not applicable for all the cases (see the next example). The latter means we need to replace AllocaOp and update its users.

Also, we could relax the GetGlobalOp verification - but it's not a good way at all and anyway we still depend on the corresponded operation verification from LLVM dialect, which may change eventually.

Example 2

void bar() {
  myfptr a[2] = {foo, foo};
}

%1 = cir.alloca !cir.array<!cir.ptr<!cir.func<!s32i (!ty_S)>> x 2>, !cir.ptr<!cir.array<!cir.ptr<!cir.func<!s32i (!ty_S)>> x 2>>, ["a"] {alignment = 16 : i64}
%2 = cir.const #cir.const_array<[#cir.global_view<@foo> : !cir.ptr<!cir.func<!s32i (!ty_S)>>, #cir.global_view<@foo> : !cir.ptr<!cir.func<!s32i (!ty_S)>>]> : !cir.array<!cir.ptr<!cir.func<!s32i (!ty_S)>> x 2>
cir.store %2, %1 : !cir.array<!cir.ptr<!cir.func<!s32i (!ty_S)>> x 2>, !cir.ptr<!cir.array<!cir.ptr<!cir.func<!s32i (!ty_S)>> x 2>>

Here we have the const array of function pointers (not lowered ones, i.e. of a wrong type) and ConstantOp which (I believe) we can not simple cast to another type, so StoreOp is wrong. So we have to rewrite it and again - rewrite AllocaOp and it's users.

Example 3

void bar() {
  T t;
  t.f = foo;
}

%2 = cir.get_global @foo : !cir.ptr<!cir.func<!s32i (!ty_S)>> 
%3 = cir.get_member %1[0] {name = "f"} : !cir.ptr<!ty_T> -> !cir.ptr<!cir.ptr<!cir.func<!s32i (!ty_S)>>>
cir.store %2, %3 : !cir.ptr<!cir.func<!s32i (!ty_S)>>

Here the struct type is involved as a storage for the function pointer. Again either cast is needed or we need to change the struct type itself.

Solution ?

So what I eager to find a good approach for the problem. Right now I think that the existing pass should match against many operations:
AllocaOp, LoadOp, StoreOp, GetMemberOp, GetGlobalOP ... - looks like its not enough just to call replaceAllUsesWith once the value type is changed. Also, the pass should perform type conversions (Example 3). I m not sure that it's a good way though - so may be you think of something else ? And the solution is simplier?
@bcardosolopes @sitio-couto

[smeenai]

CC @bruteforceboy, who's been looking at this recently too (e.g. #979)

[sitio-couto]

Hey @gitoleg !

First of all, this code won't compile at all

Function pointers are not yet handled by CC lowering. There should be an assertion noting this but it's not covering all cases.

So what I eager to find a good approach for the problem.

Function pointers are also tricky because CIR pointers are not opaque, so any related allocas will have the original function type instead of the ABI-specific one. I was looking into this since pointers are the major missing thing in CC lowering, but didn't have the time to deep dive in it.

My main idea was:

• Rewrite the function type with its ABI-specific counterpart.
• Replace any references to it (through the symbol interface).
• Bit cast any function pointer handling to and from the ABI-agnostic function type as necessary.

The last item is the trick as it may prevent us from rewriting a bunch of stuff that uses the ABI-agnostic function type (though I'm not sure if it would cover all of the cases you listed). After rewriting get_global with the ABI-specific function type, bit cast it to the ABI-agnostic one to keep the ball rolling. These pointer-to-pointer bitcasts are also dropped during lowering, so they wouldn't affect the resulting LLVM code.

Another approach would be to support opaque pointers in CIR. We keep pointee types to preserve high-level info, but considering that CC lowering is applied before moving to LLVM Dialect, and both LLVM Dialect and IR only use opaque pointers, maybe this is the way to go.

[bcardosolopes]

Another approach would be to support opaque pointers in CIR ... Bit cast any function pointer handling to and from the ABI-agnostic function type as necessary.

We've found LLVM ARM intrinsics that still require pointer info to be lowered correctly with elementtype (and the LLVM IR dialect doesn't yet support, but this is just a fixable details), I'd prefer not to run into these issues in the future because we lost information. Perhaps we could reconsider once CIR is more mature and has full coverage of the things still remaining not opaque in LLVM IR.

[bcardosolopes]

So what I eager to find a good approach for the problem. Right now I think that the existing pass should match against many operations: AllocaOp, LoadOp, StoreOp, GetMemberOp, GetGlobalOP ...

Should we create an interface that all operations that need ABI fixing can use and be tracked by?

ConstantOp which (I believe) we can not simple cast to another type

Assuming it only has one use, for ConstantOp shouldn't it be fine to just change it's type?

[gitoleg]

I thing we can close this so far, implemented in #1003 and in #1034