[PATCH] D93798: [CSKY 4/n] Add basic CSKYAsmParser and CSKYInstPrinter

Wed Dec 30 05:31:33 PST 2020

rengolin added a comment.

In D93798#2474617 <https://reviews.llvm.org/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.

You are right, it is an early refactoring issue. But @MaskRay is also right, that not doing it leads to late refactoring issues.

Because LLVM is an upstream project with lost of downstream projects depending on it, late refactoring hurts more than early refactoring.

**This is not a right vs. wrong point**. It's a //"which is less painful"// and the LLVM community has consistently chosen early refactoring over late refactoring.

The reasons are varied and extensive, but a summary would be three major points:

1. When we land the target, we want to make sure that the direction it's going is agreeable with the rest of the project.

2. Later breakages in buildbots and developer builds will increase the pressure for you to potentially have to revert your patches.

3. Early refactoring puts the pain to prove correctness on the community that proposes the patch, while late refactoring puts the pressure on everyone else, which is unfair.

Take the other back-ends that have been merged to LLVM in the past and you'll see all of them followed the same recipe. This is not an isolated opinion, it's the "status quo".

You could start a new RFC on the list to change the status quo, but that would mean your back-end effort will pause and the code will get stale.

Because it's not a right vs. wrong discussion, I predict it would lead to no consensus and no change in the status quo, so I'd advise against. But it's up to you if you want to follow that course.

> We can proposal a rough whole picture patch as reference at first to make sure the direction is right, but every small patch is changing during the review process and only suitable for newest codebase at the moment of upstream.

There are examples of this being done by major contributors and not helping at all (even if it's not meant to merge, just informational). For example ARM's SVE target and PGI's Flang front-end.

While it's good to see the overall picture, it's really hard for any one person to review the whole thing as is and take any meaningful context from it.

> It's much more burden to consider every context (A,B,C,D) for both author and reviewer.

That is true, but both author and reviewer are a very small subset of the community. We have hundreds of committers, thousands working downstream and who knows how many tests are being run.

If other changes in the project are added half-baked (assuming it will be correct in the end), it could affect your own change right now and create bugs that we can't predict and may make your life much worse.

There are a number of concurrent in-flight changes, big and small, being done to LLVM every day. If all of them land half-way, we may create a situation that we can't get out of.

True, a new back-end is a lot less dangerous than say a refactoring of the table-gen description or the pass manager, but it's still not negligible.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93798/new/

https://reviews.llvm.org/D93798