[arm] bitwise manipulation instructions #29

gnzlbg · 2017-09-20T10:26:06Z

This adds ARM's rev, rbit, clz and cls intrinsics.

The cls intrinsic currently generates bad code due to an LLVM bug, I've filled issue #30 to track it.

Closes #35 .

gnzlbg · 2017-09-21T06:53:52Z

The code-gen tests for abm, bmi and bmi2 where failing so i've fixed those here.

gnzlbg · 2017-09-21T06:55:38Z

I don't have llc for arm installed, could somebody run:

llc -march=arm -mattr=help
llc -march=armv7 -mattr=help
llc -march=aarch64 -mattr=help

and post the output here? I don't know if its possible to use #[target_feautre] to force the generation of the arm intrinsics, but if that's possible, the answer will be there.

gnzlbg · 2017-09-21T07:05:48Z

@alexcrichton the tests are failing on travis, but they pass on my machine (MacOSX). Could we also test on Mac? The problem seems to be, that on linux the assembler emits, for example, tzcnt, but on MacOS it emits tzcntw (16bit), tzcntl (32bit), tzcntq (64bit), etc. Maybe the test plugin could handle those?

gnzlbg · 2017-09-21T07:29:17Z

The last commit truncates the instructions in the assert-instr macro with the length of the expected instruction to omit the w/l/q/etc suffixes when present.

When comparing the assembly instructions against the expected instruction, depending on the platform, we might end up with `tzcntl != tzcnt`. This commit truncates the instructions to the length of the expected instruction, such that `tzcntl => tzcnt` and the comparison succeeds.

BurntSushi · 2017-09-21T10:55:09Z

@gnzlbg Thanks! So I think before we bring non-x86 intrinsics in, I'd really like to try to get CI setup to be able to test them. That's tracked in #13.

It also looks like the codegen tests are failing on Windows?

gnzlbg · 2017-09-21T11:00:28Z

It also looks like the codegen tests are failing on Windows?

Were they working on windows before?

gnzlbg · 2017-09-21T11:07:40Z

So I think before we bring non-x86 intrinsics in, I'd really like to try to get CI setup to be able to test them. That's tracked in #13.

I agree that CI for this is important. The now removed python script used to do CI for ARM by cross-compiling to arm archs and verifying the assembly. IIUC @alexcrichton will look into improving the codegen testing macros to test the codegen for arm features via cross-compiling from all hosts (even if one cannot run the run-time tests, this is better than nothing). I wanted to look next into using #[target_feature] for these ARM features but can look into fixing #13 afterward (run-time testing on ARM targets is something we want anyways).

I think that for these three things (improving the code gen testing macros), using #[target_feature] for minor ARM features, and setting up full ARM CI, having some ARM code merged that we can actually test our approaches against will be very helpful.

BurntSushi · 2017-09-21T11:12:40Z

having some ARM code merged that we can actually test our approaches against will be very helpful.

Ah yeah good point! I can be sold on that. I think I would like to make sure the CI we do have is green though. I thought Windows was green at one point? Let's wait to hear from @alexcrichton (or I can look into it, but might be a bit).

gnzlbg · 2017-09-21T11:47:49Z

@alexcrichton this is the backtrace on appveyor:

---- x86::bmi::assert_instr__mm_tzcnt_u64 stdout ----
	thread 'x86::bmi::assert_instr__mm_tzcnt_u64' panicked at 'failed to find disassembly of stdsimd::x86::bmi::_tzcnt_u64', src\libcore\option.rs:839:4
stack backtrace:
---- x86::bmi::assert_instr__tzcnt_u32 stdout ----
	thread 'x86::bmi::assert_instr__tzcnt_u32' panicked at 'failed to get symbol of function pointer: 140698179428704', assert-instr\src\lib.rs:233:16
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys_common::backtrace::_print
             at C:\projects\rust\src\libstd\sys_common\backtrace.rs:94
   1: std::panicking::default_hook::{{closure}}
             at C:\projects\rust\src\libstd\panicking.rs:380
   2: std::panicking::default_hook
             at C:\projects\rust\src\libstd\panicking.rs:391
   3: std::panicking::rust_panic_with_hook
             at C:\projects\rust\src\libstd\panicking.rs:577
   4: std::panicking::begin_panic<alloc::string::String>
             at C:\projects\rust\src\libstd\panicking.rs:538
   5: std::panicking::begin_panic_fmt
             at C:\projects\rust\src\libstd\panicking.rs:522
   6: assert_instr::assert
             at .\assert-instr\src\lib.rs:233
   7: test::{{impl}}::call_box<(),closure>
             at C:\projects\rust\src\libtest\lib.rs:141
   8: panic_unwind::__rust_maybe_catch_panic
             at C:\projects\rust\src\libpanic_unwind\lib.rs:99
---- x86::bmi::assert_instr__tzcnt_u64 stdout ----

It seems that backtrace::resolve is failing to find the symbol of a function pointer.

gnzlbg · 2017-09-21T11:51:40Z

@BurntSushi appveyor seems to be broken on master as well, I've enabled backtraces on appveyor for now, maybe we should do the same on travis.

alexcrichton · 2017-09-21T14:05:14Z

assert-instr/src/lib.rs

-                return
+            // Truncates the instruction with the length of the expected
+            // instruction: tzcntl => tzcnt and compares that.
+            if let Some(part) = part.get(0..expected.len()) {


I think this could be part.starts_with(expected)

(also thanksfor doing this!)

Thanks! I looked a lot for something like that (or contains) in the docs but couldn't find it so thought it maybe wasn't part of std. I am learning a lot with your code reviews, thank you!

alexcrichton · 2017-09-21T14:19:10Z

Hm windows is indeed previously green

gnzlbg · 2017-09-21T14:24:32Z

That is weird.

…

On Thu 21. Sep 2017 at 16:19, Alex Crichton ***@***.***> wrote: Hm windows is indeed previously green <https://ci.appveyor.com/project/rust-lang-libs/stdsimd/build/1.0.13> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NpsHqF18Mwdpr-ktXfZMDBxMtrHYNks5sknBegaJpZM4PdqMe> .

alexcrichton · 2017-09-21T14:44:31Z

Ok with the most recent logs it looks like:

x86::bmi::assert_instr__mm_tzcnt_u32 - backtrace failed, but the symbol looks present
x86::bmi::assert_instr__tzcnt_u32 - symbol not actually present, I think it was folded into _mm_tzcnt_u32
x86::bmi::assert_instr__tzcnt_u64 - symbol not actually present, I think it was folded into _mm_tzcnt_u64
x86::sse2::assert_instr__mm_adds_epi16 - backtrace failed, but the symbol looks present

Testing more...

alexcrichton · 2017-09-21T15:17:10Z

@BurntSushi also yeah I'll work on ARM CI once we get this merged, it'll be helpful to have a few examples locally to test with

alexcrichton · 2017-09-21T16:23:23Z

Alright all green now! I'll add some comments then merge

Pass the `/OPT:NOICF` flag to the linker to ensure that all functions don't get eliminated (somethign we don't want in this scenario)

gnzlbg · 2017-09-21T18:16:43Z

.appveyor.yml

+  # We don't want to do identical comdat folding as it messes up the ability to
+  # generate lossless backtraces in some cases. This is enabled by rustc by
+  # default so pass a flag to disable it to ensure our tests work ok.
+  RUSTFLAGS: -Clink-args=/OPT:NOICF


I would have never thought of this.

gnzlbg force-pushed the arm_intrinsics branch from c9bcaca to 483ca25 Compare September 20, 2017 10:29

[arm] bitwise manipulation instructions

602915a

gnzlbg force-pushed the arm_intrinsics branch from 483ca25 to 4c54e9c Compare September 21, 2017 06:53

gnzlbg mentioned this pull request Sep 21, 2017

code-gen tests should be compiled with the appropriate feature flags and platform #33

Closed

[bmi] add some more code-gen tests

799b92c

gnzlbg force-pushed the arm_intrinsics branch from 4c54e9c to 799b92c Compare September 21, 2017 07:08

gnzlbg force-pushed the arm_intrinsics branch from 2666dd1 to 75c9e0e Compare September 21, 2017 07:31

gnzlbg force-pushed the arm_intrinsics branch from 75c9e0e to 52fb0e4 Compare September 21, 2017 07:34

[appveyor] enable panic backtraces

f765407

gnzlbg force-pushed the arm_intrinsics branch from b2e7cc6 to f765407 Compare September 21, 2017 11:49

alexcrichton reviewed Sep 21, 2017

View reviewed changes

[assert-instr] simplify

2285f74

Help debug missing assembly

d116aea

alexcrichton force-pushed the arm_intrinsics branch from 3750fa6 to d116aea Compare September 21, 2017 14:32

Fix Windows MSVC CI

4fe06c1

Pass the `/OPT:NOICF` flag to the linker to ensure that all functions don't get eliminated (somethign we don't want in this scenario)

alexcrichton force-pushed the arm_intrinsics branch from 7b68771 to 4fe06c1 Compare September 21, 2017 16:24

alexcrichton merged commit 453e170 into rust-lang:master Sep 21, 2017

gnzlbg commented Sep 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[arm] bitwise manipulation instructions #29

[arm] bitwise manipulation instructions #29

gnzlbg commented Sep 20, 2017 •

edited

Loading

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017 •

edited

Loading

BurntSushi commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017 •

edited

Loading

BurntSushi commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

alexcrichton Sep 21, 2017

gnzlbg Sep 21, 2017

alexcrichton commented Sep 21, 2017

gnzlbg commented Sep 21, 2017 via email

alexcrichton commented Sep 21, 2017

alexcrichton commented Sep 21, 2017

alexcrichton commented Sep 21, 2017

gnzlbg Sep 21, 2017

[arm] bitwise manipulation instructions #29

[arm] bitwise manipulation instructions #29

Conversation

gnzlbg commented Sep 20, 2017 • edited Loading

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017 • edited Loading

BurntSushi commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017 • edited Loading

BurntSushi commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

gnzlbg commented Sep 21, 2017

alexcrichton Sep 21, 2017

Choose a reason for hiding this comment

gnzlbg Sep 21, 2017

Choose a reason for hiding this comment

alexcrichton commented Sep 21, 2017

gnzlbg commented Sep 21, 2017 via email

alexcrichton commented Sep 21, 2017

alexcrichton commented Sep 21, 2017

alexcrichton commented Sep 21, 2017

gnzlbg Sep 21, 2017

Choose a reason for hiding this comment

gnzlbg commented Sep 20, 2017 •

edited

Loading

gnzlbg commented Sep 21, 2017 •

edited

Loading

gnzlbg commented Sep 21, 2017 •

edited

Loading