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 pin..

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

I think http://lists.llvm.org/pipermail/llvm-dev/2020-August/144587.html may answer your concern. @MaskRay

take apart ELF related logics into separate patch, D86610.

Add triple test.

Address comments. Split triple work as separate part https://reviews.llvm.org/D86505.

Thank you for your review, arsenm!