simde icon indicating copy to clipboard operation
simde copied to clipboard

Fix qdmlsl instructions

Open Ryo-not-rio opened this issue 1 year ago • 3 comments

The qdmlsl instructions were implemented without any saturation. This has been fixed by utilising existing saturating instructions which are implemented correctly.

Unit tests have also been updated to test for saturation.

Ryo-not-rio avatar Jul 15 '24 16:07 Ryo-not-rio

Looks like the high_n unit tests are incorrect, looking into it

Ryo-not-rio avatar Jul 16 '24 08:07 Ryo-not-rio

Looks like the high_n unit tests are incorrect, looking into it

Thanks. I'm still seeing some test failures:

  • [x] clang (17, ubuntu-22.04, -march=native -Wno-unsafe-buffer-usage -O2) https://github.com/simd-everywhere/simde/actions/runs/10468306869/job/28988926307?pr=1195#step:9:1023
  • [x] i686-gcc11-O2 https://app.circleci.com/pipelines/github/simd-everywhere/simde/1698/workflows/8926b5aa-d8ed-4bff-9593-f67e8928897f/jobs/4801?invite=true#step-112-139088_70
  • [x] emscripten https://github.com/simd-everywhere/simde/actions/runs/10468306869/job/28988911124#step:9:2037
  • [x] gcc-qemu (-O3, 12, riscv64, riscv64, riscv64, ubuntu-22.04) https://github.com/simd-everywhere/simde/actions/runs/10468306869/job/28988921061?pr=1195#step:10:1020

There is a compile failure with MSVC, maybe that will be helpful: https://ci.appveyor.com/project/nemequ/simde/builds/50436859/job/4lpievni4ibfsa7h#L2440 (our entire MSVC CI matrix failed, not just that version: https://ci.appveyor.com/project/nemequ/simde/builds/50436859)

mr-c avatar Aug 20 '24 09:08 mr-c

Ping @Ryo-not-rio ; any update on this or your other PR?

mr-c avatar Sep 12 '24 11:09 mr-c

@Syonyk If you get a chance, your feedback here would be appreciated

mr-c avatar Jan 03 '25 21:01 mr-c

I believe these patches are correct, but were out of date with head. It looks like you've pulled them forward, so they now mostly have the changes required. I'm happy to take a closer look at this once I have my other fixes pushed, and I can test the updated files in my test suite as well.

If ryo-not-rio is not responding to requests to update tests and such, I can make a separate pull request with these changes, and add the proper test vectors. These functions were quite wrong before, though.

Syonyk avatar Jan 03 '25 21:01 Syonyk

Thank you @Syonyk ; looks like the previous compiler issues have been fixed so I've set this PR to automerge once the remaining CI checks are finished. Of course, if you still have feedback later that would be very welcome!

mr-c avatar Jan 03 '25 22:01 mr-c

Ha, I spoke too soon: MSVC is unhappy: https://ci.appveyor.com/project/nemequ/simde/builds/51262044/job/hojt0jq3999w07j7#L2451

../test/arm/neon/qdmlsl_high_lane.c(76): warning C4003: not enough arguments for function-like macro invocation 'simde_vqdmlsl_high_lane_s16'
../test/arm/neon/qdmlsl_high_lane.c(76): error C2440: 'function': cannot convert from 'simde_int16x4_t' to 'simde_int32x4_t'
../test/arm/neon/qdmlsl_high_lane.c(76): warning C4024: 'simde_vqdmlsl_s16': different types for formal and actual parameter 1
../test/arm/neon/qdmlsl_high_lane.c(76): error C2440: 'function': cannot convert from 'int' to 'simde_int16x8_t'
../test/arm/neon/qdmlsl_high_lane.c(76): warning C4024: 'simde_vget_high_s16': different types for formal and actual parameter 1
../test/arm/neon/qdmlsl_high_lane.c(76): error C2059: syntax error: ')'

mr-c avatar Jan 03 '25 22:01 mr-c

Those look like valid errors... I can certainly take a look, but it won't be until next week. I'm nearing the end of my stare-at-screen time for the week.

Syonyk avatar Jan 03 '25 23:01 Syonyk

Those look like valid errors... I can certainly take a look, but it won't be until next week. I'm nearing the end of my stare-at-screen time for the week.

No worries, I got a fix

mr-c avatar Jan 04 '25 01:01 mr-c