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

since I can't add reviewers.

Just fixed that.

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.

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.

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.

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 : )

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.

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