Support lowering VAArgOp

[ChuanqiXu9]

Close #862

I made this by introducing VaArgOp to mlir and use that op in clangir. I feel this is the most natural way to do this. I don't think we should handle different architecture in clangir. It is the job of LLVM or MLIR. I understand that we hope we don't touch any other workdirs except CIR. But it looks pretty better to do the proper thing in the proper places.

I'll try to upstream the patch to support MLIR later.

[ChuanqiXu9]

CC @ghehg @sitio-couto since I can't add reviewers.

[bcardosolopes]

since I can't add reviewers.

Just fixed that.

[bcardosolopes]

I'll try to upstream the patch to support MLIR later.

Thanks for fixing this, we indeed need that op in the LLVM dialect to make progress. We are very afraid of the technical debt though, specially the ones outside CIR stuff. The LLVM part should be done upstream first and cherry-picked here. It should also be in a different PR from the actual lib/CIR changes (so our rebaser can make into an empty commit once it kicks in against upstream, cc @lanza).

EDIT: perhaps this won't be needed at all.

[ghehg]

Thanks for working on this. This is necessary along right direction. I agree with @bcardosolopes, it might be better if we have LLVM dialect part of change landed in upstream first, then other parts of this PR can just rebase on it, and go. This way we don't have to sync changes of upstream PR and that of this PR. Let me know what you think.

[sitio-couto]

Hey @ChuanqiXu9!

I really like this approach—it's clean and a great contribution to the LLVM Dialect.

I don't think we should handle different architecture in clangir.

AFAIK, we need to handle targets in CIR to some extent, since LLVM IR doesn’t cover all the details. Still, this is a good abstraction.

One concern I have is that, for some reason, the OG Clang doesn’t use the LLVM va_arg instruction when lowering C/C++.
Maybe it's just technical debt. Maybe it's something more complex, like optimizations (choosing between reg or mem like the example above).
By emitting a different IR than the original CodeGen, we might be abstracting something we shouldn’t, like an optimization or argument-passing correctness.

Just something to keep in mind, otherwise LGTM!

[bcardosolopes]

One concern I have is that, for some reason, the OG Clang doesn’t use the LLVM va_arg instruction when lowering C/C++.

Yea, same feedback in the issue. The PR should work along the lines of Aarch64 here.

[ChuanqiXu9]

The LLVM part should be done upstream first and cherry-picked here.

Agreed. The upstreaming process looks good.

---

I didn't find the fact that we don't use va_arg for lowering X86 just now. It is a surprise to me too. I just tried the following example in x86-64:

#include <stdio.h>
#include <stdarg.h>
#include <math.h>

double stddev(int count, ...)
{
    double sum = 0;
    double sum_sq = 0;
    va_list args;
    va_start(args, count);
    for (int i = 0; i < count; ++i) {
        double num = va_arg(args, double);
        sum += num;
        sum_sq += num*num;
    }
    va_end(args);
    return sqrt(sum_sq/count - (sum/count)*(sum/count));
}

int main(void)
{
    printf("%f\n", stddev(4, 25.0, 27.3, 26.9, 25.7));
}

with clangir's pipeline and it builds and runs well. So at least va_arg can work in X86-64. I agree with it will be better to make the lowering code to be more consistent with the traditional code gen part. (but I feel it may not be a requirement or a goal to make the generated LLVM code the same. Since we will have optimizations in clang ir. So it won't be the same.)

But I am wondering if it is good to leave this patch as a fall back mechanism. I mean, this patch won't be conflicting later if we want to introduce the almost the same structure for X86-64 targets. It will be pretty helpful for people who like to run clangir on real world workloads like I am doing : )

[bcardosolopes]

but I feel it may not be a requirement or a goal to make the generated LLVM code the same

That has been part of the project's goal - it doesn't need to be exact same, but if it's deviating we should have a good reason and/or plan to fix it, I don't think it's the case here.

But I am wondering if it is good to leave this patch as a fall back mechanism

My preference is that we do the right thing, right away and do not need to revisit this, specially for x86-64, which is a super popular target. Btw, does any other target fallback to that llvm instruction?

It will be pretty helpful for people who like to run clangir on real world workloads like I am doing

It's great to hear that you're trying it on real world workloads. We're doing the same at Meta, and with that in mind we've been driving the goals and development of ClangIR. However, I don't understand why it's profitable to generate code that our vanilla clang compiler has moved away from almost a decade ago - feels like a questionable shortcut to me.

[bcardosolopes]

Reflect status from latest discussion

[ChuanqiXu9]

I sent #1088 after I failed to reopen this PR.