In D101027#2708534, @rengolin wrote:

The old 7 is now 9 and there's no 8? This is confusing.

Given that we have already merged the initial part, I think you can stop numbering the patches unless they're meant as a set, which then you start from one again (ex: 1/3, 2/3, 3/3).

Normally we would have gone all the way down small programs being generated (including ALU), but we started wrong, so let's not make it more confusing. :)

However, it's still important to know which patch depends on which, so make sure to still mark the parent/child dependencies correctly.

In D93798#2697831, @rengolin wrote:

Hi, it seems you have addressed all review comments, so this looks good to me. Thanks for all the work, and sorry it took so long.

In D93798#2694367, @rengolin wrote:

In D93798#2693768, @zixuan-wu wrote:

Could you please also accept this revision and D95029?

Hadn't noticed you had updated, sorry. Approved now.

In D95029#2694364, @rengolin wrote:

Sorry, you haven't said anything about the comments, I didn't know you had updated the patch to address them.

Phab isn't great at communication, so when you do address all comments, a simple line saying "addressing all comments" helps us know that we should look at it again.

Anyway, LGTM, thanks!

In D93798#2690896, @rengolin wrote:

In D93798#2690364, @zixuan-wu wrote:

Yep, but I was gonna make D95030 as next set of patches to handle dag codegen.

Fair enough, though that patch is probably good to go, too, with a few nits sorted.

In D93798#2688279, @rengolin wrote:

And this one, too: https://reviews.llvm.org/D95030

D93798, D94007 and D95029 are the first set of patches to review and commit together. Please have a review together, thanks.

ping..

anymore comments or LGTM?

Address comments.

In D93798#2506428, @rengolin wrote:

Hi Zixuan,

I'm still worried about the n in 4/n and 5/n. I know these are not the final patches of the back-end, but we need a minimal structure of the first batch, just like we have for m68k at the moment.

Can you isolate a first batch of patches that'd make the CSKY back-end generate ASM code for some basic code generation example (like foo(a, b) { return a+b })?

Those would be a first import, and will give reviewers the chance to understand your whole approach, not only one step into the future.

I think this is what's keeping people from reviewing your patches...

cheers,
--renato

ping...

ping..

could anybody review this patch and https://reviews.llvm.org/D94007? Gentle ping..

Yes, I agree with it's not a right vs wrong problem.

In D93798#2473722, @rengolin wrote:

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.

@MaskRay is right. Even though they do affect each other and you have to do a bit more work in refactoring them, it's a lot easier to see he effect of the early decisions on your patches on the later stages.

So far in CSKY, I have been looking at stubs and they don't look "wrong" but they also don't tell me anything about the future decisions.

You could probably compare the amount of work you do initially on a multi-patch approach with a similar amount with post-merge fixes and local refactoring.

The m64k is a good example on the benefits of having such approach, as we have already avoided some mistakes from happening in the first place.

In D93798#2472909, @MaskRay wrote:

I think the M68k has a lot which can be learned by other experimental targets. The author posted the patch series all together (1) so reviewers can review each individual patch separately and suggest code motion among patches. (2) The author shared updates on llvm-dev.

For example, if this patch were in part of a patch series, the back-and-forth style change def : ProcessorModel<"generic-csky", NoSchedModel, []>; (generic-csky->generic) could be caught earlier.

Gentle pin..

Address comments.

Is it good to anybody to LGTM?

Address comments.

In D88391#2406902, @myhsu wrote:

It would be so nice if we could start all new targets with support for Global ISel instead...

Yeah GlobalISel is definitely in our backlog/TODOs :-) And I think @RKSimon is right, GlobalISel doesn't support big-endian at this time point.

In D89180#2422723, @rengolin wrote:

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.

It would be nice to have more reviews for the CSKY back-end. Feel free to add me as a reviewer on others and I'll do my best to help.

Gentle ping.

Thank you for your review. @myhsu

Address comments

Gentle ping..

@theraven Would you like to review it as following?

Gentle ping.

Ping...

In D88466#2320949, @theraven wrote:

I lost track of the mailing list discussions, but assuming that there is consensus to accept this back end I am happy with the structure of the initial code, the size of the contributed diffs, and the engagement from the maintainer.

Address comments.

In D88466#2318573, @lattner wrote:

The general process is here: https://llvm.org/docs/DeveloperPolicy.html#adding-a-new-target

I'm sorry that I'm preoccupied with the devmtg this week, but please ping me next week if you're not getting traction, thanks!

Gentle ping...
BTW, @lattner would you know who is the decision maker that can approve a new target and review related commit?

This patch is too big to review. I will imitate the RISCV upstream process like D23560 to split this patch.

Ping... :)

Thank you so much @craig.topper for your detail review comments.

Address comments.

gentle ping...

gentle ping...

Address comments.

Address comments.

In D86610#2241879, @arsenm wrote:

In D86610#2239950, @MaskRay wrote:

This looks good to me, but I guess the target may need an official approval (from LLVM Foundation Board of Directors?) first.

I don't think it's necessary to hold up this patch on anything like that. It would be reasonable to define the reserved enum values independently of the target