[CIR][CodeGen] kr-style for function arguments

[gitoleg]

I tried to run llvm-test-suite and turned out that there are many tests fail with segfault due to old C style (let's remember Kernighan and Ritchie) . This PR fix it by the usual copy-pasta from the original codegen :)

So let's take a look at the code:

void foo(x) short x; {}
int main() {
  foo(4); 
  return 0;
}

and CIR for foo function is:

cir.func  @foo(%arg0: !s32i) {
    %0 = cir.alloca !s16i, !cir.ptr<!s16i>, ["x", init] 
    %1 = cir.cast(bitcast, %0 : !cir.ptr<!s16i>), !cir.ptr<!s32i>
    cir.store %arg0, %1 : !s32i, !cir.ptr<!s32i>
    cir.return
}

We bitcast the address (!!!) and store a value of a bigger size there.

And now everything looks fine:

cir.func no_proto  @foo(%arg0: !s32i) {
    %0 = cir.alloca !s16i, !cir.ptr<!s16i>, ["x", init] 
    %1 = cir.cast(integral, %arg0 : !s32i), !s16i
    cir.store %1, %0 : !s16i, !cir.ptr<!s16i> 
    cir.return 
} 

We truncate an argument and store it.

P.S.
The bitcast that was there before looks a little bit suspicious and dangerous. Are we sure we can do this unconditional cast while we create StoreOp ?

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[smeenai]

This is pretty nasty, nice find.

[smeenai]

OG checks that the types are different before emitting any casts. It mentions enums, but I haven't actually been able to trigger a case where that matters. (I commented out that code and check-clang still passes.) Maybe just leave a comment about that, so that it's easier to match against OG in the future?

[bcardosolopes]

Yea, looks like either assert or missing feature use cases.

[gitoleg]

added assert(!MissingFeatures::constructABIArgDirectExtend()); if you meant this enum

[smeenai]

P.S. The bitcast that was there before looks a little bit suspicious and dangerous. Are we sure we can do this unconditional cast while we create StoreOp ?

CC @bcardosolopes

[bcardosolopes]

@gitoleg thanks for the patch

P.S. The bitcast that was there before looks a little bit suspicious and dangerous. Are we sure we can do this unconditional cast while we create StoreOp ?

Perhaps we can make it a bit more strict by adding asserts for the ones we know that look absurd (like this bug)? Tricky question too: removing automatic bitcast leads to a lot of boilerplace on the call sites in CIRGen. Seems like to some extend LLVM OG codegen also does bitcasts under the hood, IIRC, which means that probably most code in CIRGen might have asserts when it differs. OTOH, keeping it automatic requires the user to sanitize and could perhaps lead to less efficient combination of casts.

I prefer the correctness angle of handling in the call sites, but seems like a lot of work if they are mostly already accounted for (which I haven't checked).

[bcardosolopes]

Need to fix few nits

[gitoleg]

@smeenai sorry for delay, done!