fix rev16 instruction

[Phosphorus15]

Hopefully this PR will fix #1127, not sure if this would affects related tests.

[ivg]

please use extract and express the semantics exactly as in the Reference Manual.

And thanks for fixing it.

[Phosphorus15]

please use extract and express the semantics exactly as in the Reference Manual.

And thanks for fixing it.

Thanks for the fast and detailed response, would you think it a good idea that I change the REV instruction to use extract too? since its current implementation is using bitmask & logand too:

| `REV, [|`Reg dest; src; cond; _|] ->
    let s = exp_of_op src in
    let i24 = int32 24 in
    let i8 = int32 8 in
    let umask = int32 0xff0000 in
    let lmask = int32 0xff00 in
    let rev =
      let open Bil in
      s              lsl i24 lor
      s              lsr i24 lor
      (s land umask) lsr i8  lor
      (s land lmask) lsl i8
    in

[ivg]

Thanks for the fast and detailed response, would you think it a good idea that I change the REV instruction to use extract too? since its current implementation is using bitmask & logand too:

Certainly, it is a good idea!

[Phosphorus15]

Please check if the fix is valid and can be merged @ivg .

[ivg]

@Phosphorus15, we use the ready for review label to make it easier to identify PRs which are, well, ready for review :)

[ivg]

Semanitcs notes:

The code is not well-typed and rev is always 0. The expression extract 7 0 s has type i8 (i.e., an 8-bit word). Thus you're shifting 8-bit words left or right by eight bits (getting zeros always). When you lor N 8-bit words, you will still get an 8-bit word. Finally, you assing the dest variable (which is a 32-bit register) an 8-bit word. And booms.

Instead of using shifts and ors, you can use extract and concat which lets you express the semantics exactly as in the reference manual.

Style notes:

1. Do not leave in ) in, ), or any other purely grammatical/service words on a single line. Each line of code, shall be a line of code. The exception is begin/end and other structural expressions, such as while, for, etc

2. Do not put superfluous parentheses. They clutter the code and confuse as it is not clear what were the author's intentions.

In this case, parenthesis around extract are acceptable, but keep in mind that function applications bind tighter than infix operators. Since in this case the infix operator is fully made of ASCII characters, it is really easy to confuse it with function application especially when there is no syntax highlighting, e.g., extract 7 0 s lsl i8 is hard to parse without highlighting, thoug it has the same (extract 7 0 s) lsl i8 meaning.

[ivg]

see the comment about REV16, it is also applicable here. And please, also just use a concatenation of 4 bytes, obtained with extract.

[ivg]

See the inline comments.

Also, it is advised to run tests locally. E.g., make test for unit tests (provided that you passed --with-tests to the ./configure script), and make check for the functional tests. This will help you to catch bugs earlier and enable shorter iterations. And remove stress from Travis, which is currently on the edge of collapsing (in other words it doesn't really work).

[ivg]

First of all, the code is not indented correctly. Second, and most importantly, you can use the [(^)] operator to make your code much more readable, e.g.,

    let rev = Bil.(extract 7 0 s ^
                   extract 15 8 s ^
                   extract 23 16 s ^
                   extract 31 24 s) in

[ivg]

the same is true for this snippet, use ^ for concatenation.

[ivg]

now it looks awesome!

[ivg]

thanks for fixing it and making the code nicer, good job!