This is an archive of the discontinued LLVM Phabricator instance.

[CSKY] Add more basic instructions including some privilege instructions
AbandonedPublic

Authored by zixuan-wu on Apr 22 2021, 1:23 AM.

Details

Summary

This patch adds more basic CSKY instructions including some privilege instructions. It involves a special operand representing register sequence like r0-r3. This operand is used at ldm/stm to load/store a sequence of register.

Diff Detail

Event Timeline

zixuan-wu created this revision.Apr 22 2021, 1:23 AM
zixuan-wu requested review of this revision.Apr 22 2021, 1:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 1:23 AM

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.

myhsu added a comment.Apr 22 2021, 9:42 AM

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

+1

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

Agree. the now 9-th patch, which contains more fundamental instructions, might be better to come before this one

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

+1 I was not able to find this patch in the review stack either

myhsu added inline comments.Apr 22 2021, 10:05 AM
llvm/lib/Target/CSKY/CSKYInstrInfo.td
464

from 426 to this line there seems to be leading whitespaces before each line, can you remove it?

477

can you remove the leading whitespaces?

zixuan-wu added a comment.EditedApr 22 2021, 11:27 PM

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.

Because there is other patch between 7 and 9, which I will make a revision later. As I plan to delay the patch of number 9 (codegen) until all basic instructions have been upstreamed, I renumber the 9 to next and next as need. Or we just abandon the prefix pattern like [CSKY 7/n]?

Because there is other patch between 7 and 9, which I will make a revision later.

I thought as much, but it's confusing because it's not public. :)

As I plan to delay the patch of number 9 (codegen) until all basic instructions have been upstreamed, I renumber the 9 to next and next as need.

The number series is an information for the reviewers to know there are more dependencies up front, on a closed series.

So, either you upload all patches at once and number them from 1 to N, or you don't number and just link them via parent/child.

Also, avoid posting patches that can't be merged on the current tree (like one after a private one). It makes review a lot harder because we don't know your internal order, only what's public.

Or we just abandon the prefix pattern like [CSKY 7/n]?

I think we better drop the numbers for now. Just make sure you have the right parent/child relationship and post patches that can be merged directly (with all the clear public dependencies merged before).

Thanks!
--renato

zixuan-wu added inline comments.Apr 25 2021, 2:49 AM
llvm/lib/Target/CSKY/CSKYInstrInfo.td
464

It's reserved for predicts at next patch.

myhsu added inline comments.Apr 25 2021, 9:34 PM
llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp
208

As clang tidy suggested, I think local variables should be capitalized

210

Is this debug message? If that's the case please use LLVM_DEBUG macro and errs() / dbgs() stream

230

Can we use std::make_pair here? The current syntax is a little too verbose

672

Can we use explicit type here?

687

ditto

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

Can we use explicit type for Ry and Rz?

zixuan-wu retitled this revision from [CSKY 7/n] Add more basic instructions including some privilege instructions to [CSKY] Add more basic instructions including some privilege instructions.May 7 2021, 7:08 PM
This comment was removed by zixuan-wu.
zixuan-wu abandoned this revision.Jul 28 2021, 2:21 AM

Because of out-of-date.