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!

This one still has some unactioned comments...

Thanks!

Seems most (all?) comments were addressed, @arsenm is there anything critical that needs done before the merge of the first batch?

Honestly, I don't see why this commit would be a problem without tests, given there aren't tests for the rest. It seems like an obvious omission to me.

In D121445#3418195, @zixuan-wu wrote:

I have met this before because the downloading of patch will ignore empty files. You can have a check that your apply does not contain new empty files in multilib_csky_linux_sdk.

I just applied this to a recent HEAD and got a few warnings. Please make sure there are no new warnings on changes / new files.

My original point was that this change is so small and unrelated to anything at the moment that it would be better for whatever s390x or i386 change that needs it, to introduce it then, not beforehand.

This is already tested by its users (via lookupTarget) in all sorts of places, but not this change in particular, which is the problem.

I'm surprised these tests are passing for you. Perhaps you're not building or running them all.

I don't know enough about your toolchain requirements, but this looks good to me.

With the nit, this looks good to me too, thanks!

In D119804#3333468, @fourdim wrote:

I'm confused about which GCC comment should be updated. Would you please point out that file or that line?
I'll plan to create a new patch to fix that.

In D119804#3333462, @rengolin wrote:

I think you forgot to upload the new diff for the GCC comment. It's looking the same as before for me.

I think you forgot to upload the new diff for the GCC comment. It's looking the same as before for me.

Nice catch, thanks!

In D115859#3312079, @rastogishubham wrote:

Hi I think I know what to do and am trying to fix it myself. Thanks for trying!

In D115859#3312022, @aprantl wrote:

If I'm making a wild guess, it doesn't look like LoongArch.def is mentioned as a textual header in module.modulemap.

In D115859#3312023, @aprantl wrote:

(The LLDB bot builds with -DLLVM_ENABLE_MODULES=On)

In D115859#3311951, @rengolin wrote:

So I committed 695b629edd03 removing that newline. It's a wild guess, it's not relevant and it shouldn't hurt, but if that's what it was (because macros are stupid), then let me know.

In D115859#3311808, @rastogishubham wrote:

Hi it seems like this patch caused an error in the lldb build machines:

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/BinaryFormat/ELF.h:877:1: error: expected identifier
#include "ELFRelocs/LoongArch.def"
^

Than you @SixWeining, the 6 patches have been merged now.

@xen0n @myhsu are you happy with this patch?

AFAICS, all issues have been fixed and the last contentious was about changing naming on the GNU side, which is completely out of scope in this series.

Sure, no problems. Let's just land all patches at once, in order, after they're all approved.

Sorry, this fell out of my radar. Looks good to me, thanks for the hard work!

In D116465#3295139, @iliya-diyachkov wrote:

So if we don't have obvious solutions for the 1st and 2nd issues, I would add this suggestion to the TODO list (we'll take it in mind during further code revisions) and go forward with the existing GlobalTypesAndRegNumPass.

In D116465#3293994, @zuban32 wrote:

Not sure how we can control the VReg IDs emitted for the types - it seems it'd require customizing IRTranslator which is not allowed by the current GlobalISel design. Am I missing something?

In D116465#3293156, @iliya-diyachkov wrote:

And as far as I understand your suggestion this de-duplicated variant will break the verifier (after generation):

func1
  %0 = OpTypeInt 32
  %1 = OpTypeInt 64
  ...
  %4 = OpIAdd %1 %2 %3
  ...
func2
  // (no definition of %1)
  ... 
  %4 = OpIMul %1 %2 %3
  ...

In D116465#3292079, @zuban32 wrote:

Which stage are you talking about here: InstPrinter, any post-InstructionSelection pass, etc? On gMIR level I don't understand how that'd work if we define VReg %1 in func f1 and then use it in both f1 and f2.

I didn't mean the duplicated types/constants would survive into codegen, but I get your meaning, it would have to be commoned up at some point.

In D116465#3290957, @zuban32 wrote:

Then let me describe the problem in more details, I'd like us all to be on the same page with this.

In D116465#3290716, @zuban32 wrote:

@rengolin what would be considered as a proper implementation?

I'm still not convinced that these passes are the right way to go, but I won't block if others think it has merit, even if just temporary, until the proper implementation emerges.

Too many comments for me to follow and check, I'll let @MaskRay and @arsenm approve this one.

@MaskRay last round?

@MaskRay last round?

@MaskRay last round?

@MaskRay last round?

Phab comments are out of place in Subtarget, so I'll let @arsenm approve this one if his comments were addressed.

All comments were addressed and this looks good to go. Thanks!

All comments seem to have been addressed and the patch itself is very mechanical in nature, so LGTM.

This looks good to me as the first patch.

In D115009#3288320, @iliya-diyachkov wrote:

In addition to the regular builds, I also checked the special ones that Fengrui Song listed earlier (all with and without -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=SPIRV). I fixed a few issues in the 3th-6th patches, so now all additional builds (as well as the regular ones) are build-able, and built llcs successfully pass LIT testing (including added SPIRV's LIT tests).

In D117793#3260730, @wintersteiger wrote:

That's correct, but without a different way to determine accessibility, we should not report likely false positives. The goal for UBSan is not to report too many false positives, and in the given circumstances, many reports would be false positives.

In D116879#3232537, @tim.schmielau wrote:

I have beefed up my testcase to demonstrate why I had to choose the _ZGVdN4vvv_sincos() variant for correctness, even though _ZGVdN4vl8l8_sincos() would be desirable from a performance perspective:
We have no control over what pointers the user is passing in in different loop iterations.

In D116879#3229779, @tim.schmielau wrote:

I haven't figured out how to link to https://bugs.llvm.org/show_bug.cgi?id=51530 so that it would automatically get closed on merging. I suppose this isn't possible anymore with bugzilla being frozen?

In D115857#3226913, @SixWeining wrote:

Those triples used in this patch are only for triple-parsing test and I think it doesn't affect triple definitions by gcc.

In D116463#3216168, @iliya-diyachkov wrote:

Yes, some missing features will be added later. What about frame lowering, the SPIRV target is very special: it uses only virtual registers. It does not operate with stack frame explicitly and does not generate a function prologue/epilogue. So it does not actually utilize frame lowering and we do not plan to add more functionality there.

You don't seem to have tests checking if the encoding of the instructions are correct when printing or parsing, leaving the Printer/Parser completely untested.

This is a much larger patch and I don't know the target well enough. I have a few general comments inline but would be nice if someone that knows the target better to have a look?

I'm guessing that the missing bits (ex. frame lowering) will be implemented in following patches.

Same as the others, looks like a standard MC patch. I do have some inline comments, though.

Nit: The name of this patch is still "[2/n]"

Can't see anything wrong with it but I don't know the SPIRV ISA, so...

Hi, this looks good as the first patch. Will keep looking for the others.

Thanks!

In D116331#3211517, @nickdesaulniers wrote:

LGTM; thanks for adding this. Reading through https://llvm.org/docs/DeveloperPolicy.html#adding-a-new-target, consider adding these numbered points under the bulleted list of points under The basic rules for a back-end to be upstreamed in experimental mode are:.

I can't see anything wrong with this patch. It seems to have all the bits to start a target in the right places.