Gentle pin...

In D98002#2657460, @evandro wrote:

In D98002#2657389, @NickHung wrote:

Adding those "sched" information into base RVV instructions seems redundant? The scheduler works on PseudoRVV instructions.

Not redundant, as some tools do use the information at this level, e.g., llvm-mca.

Moreover, this patch captures the resources in a way that it makes it easier to add latency information to specific targets later.

SchedReadWrite needs to consider pseudo instructions with LMUL? Their latency should be different.

And SEW. I'm working on this feature, of which this patch introduces the preliminary information.

BTW, I have run llvm-test-suite, it passed.

In D126451#3539656, @rengolin wrote:

This looks good to me, but wait to make sure others see it, too.

My reasons are: it is largely similar to RISCV implementation, it seems to follow what I expected of the ABI (which is similar to other targets) and has a large corpus of tests.

I can't comment on the specifics of the ABI implementation (I haven't read the ABI document *that* thoroughly), but as David said, issues will be picked up by tests until the target reaches maturity.

:)

In D126451#3539512, @DavidSpickett wrote:

Can you reupload with more context? See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

I doubt I'll be able to give any in depth review but if things look good generally I'm sure you'll find the issues through your own testing as time goes on.

In D100926#3534376, @mtrofin wrote:

In D100926#3533685, @zixuan-wu wrote:

The same work has been upstreamed at https://github.com/google/benchmark/pull/1400.
And how is this patch going? we need pull latest code to upgrade benchmark or still commit this in llvm-test-suite?
@mtrofin

It'd be best to pull from upstream, but if that is too big of a change, fine to patch here.

The same work has been upstreamed at https://github.com/google/benchmark/pull/1400.
And how is this patch going? we need pull latest code to upgrade benchmark or still commit this in llvm-test-suite?
@mtrofin

Address comments to change feature name.

Gentle pin..

Address comments.

Is there any more part of code or test I should develop in this patch together?

Address comments.

Could anybody else have a review or nominate a reviewer?

I think using two Write is not accurate because the latency are not added together but is the max one. It consumes resource simultaneously, but the expanded instructions auipc and jalr do have data dependency.
I think it should create a new SchedWrite such as WritePesudoCall and the cycle is sum up of WriteIALU and WriteJalr.

In D121445#3431569, @dyung wrote:

Hi, one of the tests you added. csky-toolchain.c seems to be failing on a Windows build bot, and from a quick look, it appears to be a path separator issue. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/216/builds/2452

In D122872#3421455, @zixuan-wu wrote:

Thank you for this patch. I was also going to draft it. Following unused variable in td file can also be removed.

diff --git a/llvm/lib/Target/CSKY/CSKYInstrFormats.td b/llvm/lib/Target/CSKY/CSKYInstrFormats.td
index 9b6ef9c..8144a50 100644
--- a/llvm/lib/Target/CSKY/CSKYInstrFormats.td
+++ b/llvm/lib/Target/CSKY/CSKYInstrFormats.td
@@ -655,7 +655,7 @@ class R_Z_1<bits<6> sop, bits<5> pcode, string op>

 // Format< OP[6] | RZ[5] | 00000[5] | SOP[6] | PCODE[5] | 00000[5] >
 // Instructions:(2) clrf32, clrt32
-class R_Z_2<bits<6> sop, bits<5> pcode, string op, list<dag> pattern>
+class R_Z_2<bits<6> sop, bits<5> pcode, string op>
     : CSKY32Inst<AddrModeNone, 0x31, (outs GPR:$rz),
     (ins CARRY:$ca, GPR:$false), !strconcat(op, "\t$rz"), []> {
   bits<5> rz;
diff --git a/llvm/lib/Target/CSKY/CSKYInstrInfo.td b/llvm/lib/Target/CSKY/CSKYInstrInfo.td
index 28e364c..300ecce 100644
--- a/llvm/lib/Target/CSKY/CSKYInstrInfo.td
+++ b/llvm/lib/Target/CSKY/CSKYInstrInfo.td
@@ -734,8 +734,8 @@ let Predicates= [iHasE2] in {

 let Predicates = [iHas2E3] in {
   def MVCV32 : R_Z_1<0x1, 0x10, "mvcv32">;
-  def CLRF32 : R_Z_2<0xB, 0x1, "clrf32", []>;
-  def CLRT32 : R_Z_2<0xB, 0x2, "clrt32", []>;
+  def CLRF32 : R_Z_2<0xB, 0x1, "clrf32">;
+  def CLRT32 : R_Z_2<0xB, 0x2, "clrt32">;
 }

Thank you for this patch. I was also going to draft it. Following unused variable in td file can also be removed.

Is CSKY target enabled as default in lld building even it's an experimental target?

In D121445#3416518, @rengolin wrote:

There is also a test error:

******************** TEST 'Clang :: Driver/csky-toolchain.c' FAILED ********************
...
/home/rengolin/devel/llvm-project/clang/test/Driver/csky-toolchain.c:16:24: error: C-CSKY-LINUX-MULTI: expected string not found in input
// C-CSKY-LINUX-MULTI: "{{.*}}/Inputs/multilib_csky_linux_sdk/lib/gcc/csky-linux-gnuabiv2/6.3.0/../../..{{/|\\\\}}..{{/|\\\\}}csky-linux-gnuabiv2/bin{{/|\\\\}}ld"

                       ^

I'm guessing this is the path of a local sysroot you have on your machine?

If possible, try to get a new environment (container, VM, alternative machine) and make sure a clean build still passes the tests.

Address comments.

In D121445#3388842, @rengolin wrote:

I'm surprised these tests are passing for you. Perhaps you're not building or running them all.

To make sure you're running your tests, you need to build both clang and llvm (-DLLVM_ENABLE_PROJECTS=clang) and run ninja/make check-all.

You can also run lit directly on each test, but I can't remember how to do that now...

I have updated the patch. Any more comments?

Address comments.

In D119917#3348237, @shafik wrote:

Looks like this PR broke the green dragon bot, see: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/41771/consoleFull#10309019849ba4694-19c4-4d7e-bec5-911270d8a58c

Address comments.