Skip to content

Conversation

@lukel97
Copy link
Contributor

@lukel97 lukel97 commented Dec 2, 2024

This was pointed out in #118283 (comment). We cannot move these between vtype definitions as they depend on SEW and require vill to be clear.

This was pointed out in llvm#118283 (comment). We cannot move these between vtype definitions as they depend on SEW and require vill to be clear.
@llvmbot
Copy link
Member

llvmbot commented Dec 2, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Luke Lau (lukel97)

Changes

This was pointed out in #118283 (comment). We cannot move these between vtype definitions as they depend on SEW and require vill to be clear.


Full diff: https://github.com/llvm/llvm-project/pull/118414.diff

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfoV.td (+1-1)
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoV.td b/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
index 8e0c4826ac00de..6506b6746b1517 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
@@ -1726,7 +1726,7 @@ foreach n = [1, 2, 4, 8] in {
   def VMV#n#R_V  : RVInstV<0b100111, !add(n, -1), OPIVI, (outs vrc:$vd),
                            (ins vrc:$vs2), "vmv" # n # "r.v", "$vd, $vs2">,
                    VMVRSched<n> {
-    let Uses = [];
+    let Uses = [VTYPE];
     let vm = 1;
   }
 }

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lukel97 lukel97 requested a review from mshockwave December 3, 2024 01:14
@lukel97
Copy link
Contributor Author

lukel97 commented Dec 3, 2024

I had to update the llvm-mca tests, I'm not sure why adding a use affects the number of cycles. cc @mshockwave

@preames
Copy link
Collaborator

preames commented Dec 3, 2024

I had to update the llvm-mca tests, I'm not sure why adding a use affects the number of cycles. cc @mshockwave

LGTM as well, the mca change is at least reasonable if we're modeling a new runtime dependency which wasn't previously being modeled.

@topperc
Copy link
Collaborator

topperc commented Dec 3, 2024

I had to update the llvm-mca tests, I'm not sure why adding a use affects the number of cycles. cc @mshockwave

LGTM as well, the mca change is at least reasonable if we're modeling a new runtime dependency which wasn't previously being modeled.

Looking at the test. There are vsetvlis interleaved with vmv. So I assume llvm-mca is now waiting for the preceding vsetvli before it can start a vmv.

@mshockwave
Copy link
Member

mshockwave commented Dec 3, 2024

I had to update the llvm-mca tests, I'm not sure why adding a use affects the number of cycles. cc @mshockwave

LGTM as well, the mca change is at least reasonable if we're modeling a new runtime dependency which wasn't previously being modeled.

Looking at the test. There are vsetvlis interleaved with vmv. So I assume llvm-mca is now waiting for the preceding vsetvli before it can start a vmv.

Correct, this is the timeline before:

Index     0123456789          0123456789          0123456789          0123456789

[0,0]     DeER .    .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, mf8, tu, mu
[0,1]     DeeER.    .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,2]     D=eER.    .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, mf4, tu, mu
[0,3]     .DeeER    .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,4]     .D=eER    .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, mf2, tu, mu
[0,5]     .D=eeER   .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,6]     . D=eER   .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, m1, tu, mu
[0,7]     . D=eeER  .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,8]     . D==eER  .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, m1, tu, mu
[0,9]     .  D=eeER .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,10]    .  D==eER .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, m2, tu, mu
[0,11]    .  D==eeER.    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16

And this is the one after:

Index     0123456789          0123456789          0123456789          0123456789

[0,0]     DeER .    .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, mf8, tu, mu
[0,1]     D=eeER    .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,2]     D=eE-R    .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, mf4, tu, mu
[0,3]     .D=eeER   .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,4]     .D=eE-R   .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, mf2, tu, mu
[0,5]     .D==eeER  .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,6]     . D=eE-R  .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, m1, tu, mu
[0,7]     . D==eeER .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,8]     . D==eE-R .    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, m1, tu, mu
[0,9]     .  D==eeER.    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16
[0,10]    .  D==eE-R.    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vsetvli    zero, zero, e8, m2, tu, mu
[0,11]    .  D===eeER    .    .    .    .    .    .    .    .    .    .    .    .    .   .   vmv1r.v    v8, v16

vmvNr no longer can be issued at the same cycle with the preceding vsetvli.

@wangpc-pp
Copy link
Contributor

vmvNr no longer can be issued at the same cycle with the preceding vsetvli.

(Disclaimer: this is just my imagination, real microarchitecture may not be like this)
For this part, the hardware may issue vmvNr (using implementation-specific EEW, for example, EEW=8) before vtype is ready and check vill when committing. And also, there are some existing hardwares that don't trap.
What I am trying to say is, the modeling of side effect and instruction scheduling in compiler may differ from hardware behavior, and that may cause sub-optimal instruction sequences after scheduling. This may not be a big problem, we can revisit it if we do see some real issues it causes.

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@lukel97 lukel97 merged commit 5cd3e97 into llvm:main Dec 4, 2024
8 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Dec 4, 2024

LLVM Buildbot has detected a new failure on builder libc-x86_64-debian-fullbuild-dbg-asan running on libc-x86_64-debian-fullbuild while building llvm at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/171/builds/11573

Here is the relevant piece of the build log for the reference
Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py ...' (failure)
...
[       OK ] LlvmLibcMMapTest.Error_InvalidSize (5 us)
Ran 2 tests.  PASS: 2  FAIL: 0
[932/1099] Running unit test libc.test.src.sys.mman.linux.posix_madvise_test
[==========] Running 2 tests from 1 test suite.
[ RUN      ] LlvmLibcPosixMadviseTest.NoError
[       OK ] LlvmLibcPosixMadviseTest.NoError (32 us)
[ RUN      ] LlvmLibcPosixMadviseTest.Error_BadPtr
[       OK ] LlvmLibcPosixMadviseTest.Error_BadPtr (4 us)
Ran 2 tests.  PASS: 2  FAIL: 0
[933/1099] Running unit test libc.test.src.sys.mman.linux.process_mrelease_test
FAILED: projects/libc/test/src/sys/mman/linux/CMakeFiles/libc.test.src.sys.mman.linux.process_mrelease_test /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-fullbuild-dbg-asan/build/projects/libc/test/src/sys/mman/linux/CMakeFiles/libc.test.src.sys.mman.linux.process_mrelease_test 
cd /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-fullbuild-dbg-asan/build/projects/libc/test/src/sys/mman/linux && /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-fullbuild-dbg-asan/build/projects/libc/test/src/sys/mman/linux/libc.test.src.sys.mman.linux.process_mrelease_test.__build__
[==========] Running 3 tests from 1 test suite.
[ RUN      ] LlvmLibcProcessMReleaseTest.NoError
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-fullbuild-dbg-asan/llvm-project/libc/test/src/sys/mman/linux/process_mrelease_test.cpp:44: FAILURE
Failed to match LIBC_NAMESPACE::process_mrelease(pidfd, 0) against Succeeds().
Expected return value to be equal to 0 but got -1.
Expected errno to be equal to "Success" but got "No such process".
[  FAILED  ] LlvmLibcProcessMReleaseTest.NoError
[ RUN      ] LlvmLibcProcessMReleaseTest.ErrorNotKilled
[       OK ] LlvmLibcProcessMReleaseTest.ErrorNotKilled (801 us)
[ RUN      ] LlvmLibcProcessMReleaseTest.ErrorNonExistingPidfd
[       OK ] LlvmLibcProcessMReleaseTest.ErrorNonExistingPidfd (9 us)
Ran 3 tests.  PASS: 2  FAIL: 1
[934/1099] Running unit test libc.test.src.sys.mman.linux.mremap_test
[==========] Running 2 tests from 1 test suite.
[ RUN      ] LlvmLibcMremapTest.NoError
[       OK ] LlvmLibcMremapTest.NoError (34 us)
[ RUN      ] LlvmLibcMremapTest.Error_InvalidSize
[       OK ] LlvmLibcMremapTest.Error_InvalidSize (17 us)
Ran 2 tests.  PASS: 2  FAIL: 0
[935/1099] Running unit test libc.test.src.sys.mman.linux.remap_file_pages_test
[==========] Running 3 tests from 1 test suite.
[ RUN      ] LlvmLibcRemapFilePagesTest.NoError
[       OK ] LlvmLibcRemapFilePagesTest.NoError (147 us)
[ RUN      ] LlvmLibcRemapFilePagesTest.ErrorInvalidFlags
[       OK ] LlvmLibcRemapFilePagesTest.ErrorInvalidFlags (57 us)
[ RUN      ] LlvmLibcRemapFilePagesTest.ErrorInvalidAddress
[       OK ] LlvmLibcRemapFilePagesTest.ErrorInvalidAddress (4 us)
Ran 3 tests.  PASS: 3  FAIL: 0
[936/1099] Running unit test libc.test.src.string.memmove_test.__unit__
[==========] Running 7 tests from 1 test suite.
[ RUN      ] LlvmLibcMemmoveTest.MoveZeroByte
[       OK ] LlvmLibcMemmoveTest.MoveZeroByte (5 us)
[ RUN      ] LlvmLibcMemmoveTest.DstAndSrcPointToSameAddress
[       OK ] LlvmLibcMemmoveTest.DstAndSrcPointToSameAddress (3 us)
[ RUN      ] LlvmLibcMemmoveTest.DstStartsBeforeSrc
[       OK ] LlvmLibcMemmoveTest.DstStartsBeforeSrc (2 us)
[ RUN      ] LlvmLibcMemmoveTest.DstStartsAfterSrc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants