Page MenuHomePhabricator

[CSKY 4/n] Add basic CSKYAsmParser and CSKYInstPrinter
ClosedPublic

Authored by zixuan-wu on Dec 24 2020, 1:40 AM.

Details

Summary

This basic parser will handle basic instructions with register or immediate operands.
With the addition of CSKYInstPrinter, we can now make use of lit tests.

Diff Detail

Event Timeline

zixuan-wu created this revision.Dec 24 2020, 1:40 AM
zixuan-wu requested review of this revision.Dec 24 2020, 1:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 24 2020, 1:40 AM

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.

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.

The whole picture can be referred here https://reviews.llvm.org/D86269.
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.
Yes, it's good to sync the process of development in llvm-dev maillist phase by phase

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.

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.

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

For example, the development process is A->B->C. We can/should not make the structure of B is good to C as we lookup ahead the future patch(C), because B is in codebase of A.
What's more, if we have D following C (C->D), the early review of D is meaningless as B is changing and the change of C is much more. It's much more burden to consider every context (A,B,C,D) for both author and reviewer.

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.
  1. Later breakages in buildbots and developer builds will increase the pressure for you to potentially have to revert your patches.
  1. 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.

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

I want to say every patch when it's upstreamed is not half-baked or land half-way. It's good enough for current codebase with test cases so that it's workable for downstream.
There is just no perfect infra which is needed and introduced by the later patch/feature.

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.

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.

Awesome! Thank you!

zixuan-wu added a comment.EditedJan 4 2021, 7:55 PM

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

zixuan-wu updated this revision to Diff 317494.Jan 19 2021, 2:06 AM

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

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

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

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

Sounds great, thanks @zixuan-wu!

@MaskRay should be better now to have another look, thanks!

MaskRay added inline comments.Jan 20 2021, 11:06 PM
llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
79

Delete CSKYOperand -
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
"Don’t duplicate the documentation comment in the header file ..."

82

Delete the blank line

169

"Don’t duplicate the documentation comment in the header file ..."

171

"Don’t duplicate the documentation comment in the header file ..."

195

can use one OS

238

Complete sentences end with periods.

This applies to many other places.

290

May use D94649

435

Period.

llvm/lib/Target/CSKY/MCTargetDesc/CSKYInstPrinter.cpp
102

No newline at end of file

llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCTargetDesc.cpp
72

clang-format prefers /*TuneCPU=*/

llvm/test/MC/CSKY/csky-invalid.s
1 ↗(On Diff #317494)

Instead of a separate error test, consider merging into the positive test and use .ifdef ERR (you can find several examples in test/MC added by me)

4 ↗(On Diff #317494)

[[@LINE]] is deprecated lit syntax. Use [[#@LINE]]

llvm/test/MC/CSKY/csky-valid.s
1 ↗(On Diff #317494)

Instead of csky-valid.s, you may pick basic.s. every file in this directory is C-SKY related so the name csky.s does not convey additional information. -valid can be omitted because most tests are positive tests.

MaskRay added inline comments.Jan 20 2021, 11:09 PM
llvm/test/MC/CSKY/csky-valid.s
2 ↗(On Diff #317494)

CHECK-ASM,CHECK-ASM-AND-OBJ was probably copied somewhere else. MCAsmStreamer and MCObjectStreamer are different enough that sometimes you want to test both llvm-mc and llvm-mc -filetype=obj ... | llvm-objdump -d

If the disassembly part hasn't been implemented, don't add CHECK-ASM-AND-OBJ

zixuan-wu updated this revision to Diff 318131.Jan 21 2021, 1:05 AM

Address comments.

zixuan-wu marked 10 inline comments as done.Jan 21 2021, 1:06 AM

anymore comments or LGTM?

anymore comments or LGTM?

@zixuan-wu, like other multi reviews, wait until all pending patches are approved to commit all of them in one go.

This allows discussion in other patches to happen and maybe change existing approvals if issues are found.

I'll let @MaskRay approve this one.

MaskRay added inline comments.Jan 28 2021, 11:43 AM
llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
125

Is isMem unimplemented? If yes, deferring to a subsequent patch is better.

284

The convention is to have a space after :

llvm/test/MC/CSKY/basic.s
2

The joined line is sufficiently short, no need for wrapping.

99

If you use ## for non-RUN non-CHECK comment markers, please stick with the rule consistently for subsequent comments.

zixuan-wu added inline comments.Thu, Apr 1, 10:22 PM
llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
125

It's implement of pure virtual function, I am afraid it can not ignore.

zixuan-wu updated this revision to Diff 334899.Thu, Apr 1, 11:13 PM
zixuan-wu marked 3 inline comments as done.

Hi @zixuan-wu,

There are still some comments on the remaining patches in this set that need addressing before all of them can be committed as a whole.

Addressing all the reviews on the set increases the chances of people checking back (otherwise, it looks to us that you're still working on them).

Thanks!
--renato

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

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

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.

zixuan-wu added a comment.EditedThu, Apr 15, 11:20 PM

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.

Could you please also accept this revision and D95029?

Could you please also accept this revision and D95029?

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

zixuan-wu marked an inline comment as done.Sun, Apr 18, 7:39 PM

Could you please also accept this revision and D95029?

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

Sorry for the confuse. Thank you for your review.
How about this patch ?

rengolin accepted this revision.Mon, Apr 19, 12:45 AM

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.

This revision is now accepted and ready to land.Mon, Apr 19, 12:45 AM

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.

As our internal codebase is changing and not stable, I delayed the process of upstreaming to community about CSKY backend. So it lasts too long, and the process will be restarting and going further now.
Thank for your support.

This revision was landed with ongoing or failed builds.Tue, Apr 20, 12:38 AM
This revision was automatically updated to reflect the committed changes.