In D95386#2522648, @mjp41 wrote:

@ezhulenev I think I can see what the fix should be. Does the check-mlir cover this example? Or is there additional testing I should do?

This is the error:

In D93798#2518757, @zixuan-wu wrote:

anymore comments or LGTM?

Overall, I think this is great. It answers all questions I've seen posed on the RFCs and is well aligned with the tiers policy, in text and intent.

In D93798#2509226, @zixuan-wu wrote:

Yes, a first batch of patches to reach the point that it can generate first ALU asm codegen. In all, I propose following patches to construct the first batch.

https://reviews.llvm.org/D93798
https://reviews.llvm.org/D94007
https://reviews.llvm.org/D95029
https://reviews.llvm.org/D95030

Hi Zixuan,

In D88385#2506204, @RKSimon wrote:

The plan is for the entire patch series to be accepted before any are pushed.

In D88385#2502299, @myhsu wrote:

Absolutely. I agree this problem should be solve before graduate from experimental target. I will create these bugs soon

In D88385#2501781, @jrtc27 wrote:

That sounds reasonable (but +1 to @RKSimon 's comment about reviewing all this "special sauce" complexity before graduating from experimental)

In D88385#2498908, @jrtc27 wrote:

Yeah my concern is not about m68k doing weird things, it's about the code that's added in the target-independent places to support m68k that I am most concerned with, as it affects every target regardless of whether m68k is enabled.

In D94451#2491668, @tstellar wrote:

If this pitch is accepted, then what are the next steps required in order for bazel or another build system to be added to tree? For example, can they automatically be added if they meet the support policy requirements or does there need to be some kind of community ack for? I think the pitch needs to be more clear about what this process is.

Also, please use diff -U9999 as documented: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

In D88385#2476435, @RKSimon wrote:

Why would refactoring TSFlags into a int-ptr union just for M68k be better than adding the CodeBeads functionality just for M68k?

In D92748#2478802, @kaz7 wrote:

Thank you for the response. Your description is correct. That is what I tried to describe. Thanks.

In D92748#2454376, @kaz7 wrote:

@compnerd , I have a question. I'm thinking that you might misunderstand my explanations. VE is almost merged. I can compile llvm-project using already upstreamed llvm/clang. Now, in order to continue the upstreaming, I need crtbegin.o and crtend.o in compiler-rt. It is a reason why I submit this patch. Didn't you misunderstand the situation? Thanks!

In D93798#2475498, @zixuan-wu wrote:

I think it's not worthwhile to spend too much time to discuss this open topic. Let's come back to review this and following patches, please. Thank you! I can make few diff files based on this one.

In D93798#2474617, @zixuan-wu wrote:

I think it's early-refactoring issue. We should assume the current patch is good in current codebase context, or we will re-review many times.

In D93798#2473365, @zixuan-wu wrote:

I don't think too many patches on the fly is good to upstream because they influence each other and need change on the way of review.

LGTM, thanks!

This LGTM now, thanks!

LGTM too, thanks!

Looks like a standard mock implementation, basic infrastructure. I don't see anything wrong with it but I've also been far from this area for too long, so I'll let other people review it properly and approve.

LGTM too, thanks!

This looks good to me but I'll let other people review again and approve. Thanks!

@craig.topper are you happy with the changes?

In D88385#2442226, @RKSimon wrote:

Does anyone have any more comments? Otherwise I think we can provisionally accept this.

In D88391#2441942, @zixuan-wu wrote:

Hmm, I am just curious about that whether GlobalISEL is mature enough to use as default instruction selection method without implementing SelectionDAG. As a newer backend, I really would like to drop legacy stuff at start if it works.

Thanks for the changes. This looks good to me but I'll let others check again and approve.

I can't possibly comment on the ISA implementation, but this looks like a standard table-gen set of files to me. They also don't affect anything else outside of CSKY, so LGTM.

You should at least get @craig.topper's feedback, given this is a significant change in the x86 backend.

In D88389#2401213, @myhsu wrote:

Oh, from the discussion on mailing list I thought this is a requirement.

In D88389#2399389, @RKSimon wrote:

Is it worth trying to avoid adding/deleting the *Dummy* files in the patch series? @rengolin any thoughts ?

Nice set of tests, well separated, not a lot of pattern-match on the FileCheck side, which is good, asm and object emission.

I only had a very quick overview and this looks fine.

A few nits, but this is looking good.

Hi @fpetrogalli, the document is so dense that it took me a while to check the macros and I was still wrong.

It's looking a bit redundant to me, but I wanted to make sure we have the link on all three sections in case people are given a link (or navigate from the table-of contents) and don't see the sections above.

Sorry, forgot the review text on the commit message:
Closed by 25ba6b2bcd1

Thanks Chris! Will do.

In D90761#2380282, @mehdi_amini wrote:

LGTM from me on the principle, I didn't get into detailed wordsmith. Overall I would have been more concise and lay down the general principle rather than trying to prescribe this level of details (like the "Reinstatement" section for example), I believe that's also more in line with how we've been operating / documenting things in the past, but I don't really mind it either.
I suspect we can incrementally fix/improve anything as we go as well.

• Include a short phrase on inclusion policy, as requested.
• Fixed some typos

Simple and straightforward, with documentation! :)

Nice change, simpler to read and easier to get right. LGTM, thanks!

Sorry, s/George/Geoffrey/. :S

• Adding inclusion policy section
• Reword phrase in deprecation policy

In D90761#2374520, @GMNGeoffrey wrote:

Overall LGTM, with some wording nits. One thing I think is missing is guidance for how a peripheral component gets added. Experimental targets already have their own documentation, and my impression is that editor bindings are just a matter of sending a patch (which seems reasonable to me). What about something like:

To add a new peripheral component not covered under an existing policy, send an RFC to the appropriate dev list proposing its addition and explaining how it will meet the support requirements listed here.

Rewriting some confusing paragraphs.

Updating simple suggestions / fixups.

• Adding page link to "Getting Involved" page.
• Fixing some formatting errors (Sphinx reports no warnings on my box).

The change looks good to me but @gkistanova usually handles buildbot operations, so I don't want to approve before she has time to look at it.

W00t! FreeBSD FTW!

Accepting again. I particularly like @gribozavr2's suggestion. LGTM, thanks!

I have read all of the comments in this review and have looked at all the other pending reviews because of this and I still see strong disagreement on the design and implementation.

In D89995#2354061, @nhaehnle wrote:

Not everybody is necessarily aware of a 4 year old email thread;

In D89995#2352053, @nhaehnle wrote:

Considering that you are doing this in response to D83088, I think we have pre-existing evidence that this isn't workable as-is.

I don't want to "take sides" and I'm only looking at this review from what it is and my own experience in the community. But looking at D83088, I see a somewhat problematic review.

This is a great policy, thanks @silvas and @mehdi_amini.

@MaskRay you seem to have fixed partially, and @thakis solution is better than what I did. To avoid a similar clash, I'll revert my change and if it breaks again, you can fix it directly.

In D85024#2328693, @tinti wrote:

@MaskRay ping?

@MaskRay are you happy with the latest changes? If so, please change your review. Thanks!

In D88385#2307461, @myhsu wrote:

Interesting...the patch I updated yesterday had gone through clang-format. And I didn't see any lint warning here on Phabricator. Maybe I should run again.

Adding @craig.topper as the owner of the x86 back-end, to check the implementation of the CodeBeads concept. I have mainly worked with RISC back-ends, so it's not my area. Craig, please feel free to add whoever you think is best to review this.

There are plenty of lint checks that need addressing. Some are false positives and those are obvious cases, but the rest should be followed.

No problems, feel free to close, thanks!