In D89322#4188849, @dmgreen wrote:

This too old to be useful now and I don't have any plans to work on it in the near term. (It would be good to see improvements though, where the vplan is costed more directly as opposed continuing to go through the IR instructions).

IIRC, "abi" used to be what Arm called, but "env" is equally good. And it it's consistent with Triple.h, even better. LGTM, thanks!

In D144692#4183069, @mehdi_amini wrote:

I don't believe there is any rule like that: I merge my patches as they get approved in general. We mostly need to agree about the overall design, then we can develop incrementally in tree. Stacked patches are mostly a convenient way of streamlining the development and review process.

In D144692#4182393, @math-fehr wrote:

That makes sense, I'm adding this in a follow-up patch.

PS: I have just merged the dependency D144690.

In D144692#4179101, @math-fehr wrote:

To further simplify this patch (and the registration one), I removed any_of from irdl.
I'll add it in the patch right after the registrations one.

However, I kept the mentions to any_of in the documentation, if that is fine for you, to not rewrite almost all the code snippets (since all of them used any_of).

This looks good to me. @fhahn ?

In D144101#4174508, @fhahn wrote:

It would probably be even better to make the threshold an argument (perhaps pass the threshold optionally like --filter-short=N), with the default being 1.0?

Overall looks good to me, thanks!

Nice work! I have some comments inline, mostly to understand the dialect.

Running with ninja without -j means it will take more than the number of cores by default. If you want a single core, you need to specify -j1.

Ok, created a proposal to split builtins: https://discourse.llvm.org/t/proposal-split-built-ins-from-the-rest-of-compiler-rt/67978

Just to make sure there's no panic, I'm blocking the revision until more people looked over. I'll remove once we have a number of approvals from people in different positions.

In D141836#4080877, @cjdb wrote:

+1 to trying to add these to libgcc instead of removing from compiler-rt.

In D141836#4074521, @echristo wrote:

The right move here is to check about muloti, not disable it unless the
aarch64 backend doesn't need it.

In D141836#4073296, @sebastianpoeplau wrote:

From reading the thread on https://github.com/llvm/llvm-project/issues/16778 (which is closed, although the issue doesn't seem resolved), this is a stop-gap for toolchains that don't include an implementation for __muloti4. An implementation seems to exist in compiler-rt, but compiler-rt isn't linked with by default. It seems unfortunate that there is no way to still emit a call to __muloti4 when we do link with compiler-rt, but from reading that thread there is currently no way to tell LLVM that an implementation of __muloti4 is available.

That's my understanding as well.

Nice! Thanks for the detailed reporting. LGTM.

LGTM, thanks!

Thanks everyone, this looks like a good state and consensus. @SixWeining please check with @tstellar directly before merging, to make sure you get the right time for the release.

Thanks @chelini, LGTM now, but I'll let others make the final approval.

Thank you for your continued contributions, @SixWeining!

Make sure you add a note to the changelog, so people know you've changed all the enums involved.

Small nit, but this looks great, thanks!

In D138672#3980995, @SixWeining wrote:

If self-maintained buildbot can meet the requirement for a back-end to be promoted to official, we would have a try.

Galina may be on extended holiday.

LGTM, thanks! Please check with @gkistanova before merging.

LGTM, thanks!

LGTM, thanks! @chelini any comments?

LGTM too, with a nit.

Simplified what's exposed to the methods, keeping the Options on the CPP file. Rebased and all tests pass.

@chelini

As per that thread, indeed MCJIT is on the way out and you should go for OrcJIT if you want JIT support in LoongArch.

All my comments addressed, if other people are happy, I'm also happy. Thanks!

Looks good, with some nits. Thanks!

In D134451#3817114, @akuegel wrote:

@chelini if you roll forward, please consider updating the Bazel BUILD files like I did in the two revisions mentioned above

Honestly, there isn't much meat here, other than the decision to support and the name bikeshedding, both of which I think are well covered in the RFC.

Thanks, looks good to me!

I can't vouch for your ABI correctness, but you seem to have a lot of tests covering what you claim to have implemented, so looks good to me.

I think we can safely say that we care less about jazelle than we care about armv1/2/3, so feel free to ignore that, too.

LGTM, thanks!

A few nits, but looks good to me, thanks!

Agree. Even 10 years ago we made the concerted effort not to care about pre-v4, so I'd be a little surprised if people are actually using modern clang to target those platforms.

W00t!

Looks simple enough!

This looks great, thanks!

No requests, this looks simple enough for me, thanks!

Makes sense, I have no additional comments. LGTM, thanks!

I don't know much about DX but this looks like object emission to me. :)

I don't know much about DirectX but I'm assuming all of those (what, dialects?) will be coming through the same printers/parsers, right?

Is this the only thing the Asm Printer will ever do? Does it mean you'll never emit asm files or is there another way to get it out (and if there is, why aren't the constants there, too)?

This looks fine to me. I don't see how this can be tested and if it never will have functionality in the future, there's not point in even trying.

In D126889#3570043, @simoll wrote:

We got one accept and no opposition. This passes all testing locally. If pre-merge checks are ok (except for unrelated breakage), I will go forward and commit this.

In D127199#3564217, @xry111 wrote:

Does anyone have any idea about "why the CI is failing"? It seems happening for this batch of LoongArch patches and also one MIPS patch I've reviewed today. I guess something is wrong in the CI system or LLVM codebase.

LGTM, super simple, too.

LGTM, simple and straightforward.

Perhaps wait a few days to make sure other people have time to comment, too.

As with the alternative, this needs more agreements, but in this case, it touches more files and the probability it will create merge conflicts with other people's in-flight work increases.

I don't know how people feel nowadays about clang-formatting an entire file.

In D126451#3541590, @zixuan-wu wrote:

I don't think it is largely similar to RISCV implementation except for the code structure and test reuse. And the test result is big different.

This looks good to me, but wait to make sure others see it, too.

First, I think this is a good idea and can eventually mitigate the general problem of intrinsics vs. instructions in other LLVM passes.

In D125404#3508754, @andreytr wrote:

And what about build failures? It seems, these are not related to my patch...

In D125404#3508472, @andreytr wrote:

Concerning "[[#]] instead of {{[0-9]+}}":
This style {{[0-9]+}} is widely used in SPIRV-LLVM-Translator's tests repository. So, it has to change nearly 150-200 converted tests in LLVM-SPIRV-Backend' tests repository. (?) Moreover, it will break link to original source SPIRV-LLVM-Translator's tests.
Is it required?

In D123988#3463790, @MaskRay wrote:

https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted says "If it is likely that others will want to review a recently-posted patch, especially if there might be objections, but no one else has done so yet, it is also polite to provide a qualified approval (e.g., “LGTM, but please wait for a couple of days in case others wish to review”). If approval is received very quickly, a patch author may also elect to wait before committing (and this is certainly considered polite for non-trivial patches). Especially given the global nature of our community, this waiting time should be at least 24 hours. Please also be mindful of weekends and major holidays."

LGTM, thanks!

Looks reasonable to me. Some nits on file naming and definition placement. Thanks!

Thanks @iliya-diyachkov, it looks cleaner, even if still very long. A table generated output will hopefully fix this soon.

Apologies, it seems I was looking into a very old version of the review!