Review Board 2.0.15


x86: fix Mul1u instruction

Review Request #3800 - Created Feb. 1, 2017 and updated

Information
Tony Gutierrez
gem5
default
Reviewers
Default
Changeset 11778:c76b78110490
---------------------------
x86: fix Mul1u instruction

the Mul1uFlags and Mul1u instructions perform
the 64b multiplication using only 64b registers, however the
method used causes the high 64b to be corrupted for certain
inputs. here we fix the computation.

   
Review request changed
Updated (Feb. 3, 2017, 4:13 p.m.)

Summary:

-x86: fix Mul1u and Mul1s instructions
+x86: fix Mul1u instruction

Description:

~  

Changeset 11970:b05562ad3992

  ~

Changeset 11778:c76b78110490

   
~  

x86: fix Mul1u and Mul1s instructions

  ~

x86: fix Mul1u instruction

   
~  

the Mul1uFlags, Mul1u, Mul1sFlags, and Mul1s instructions perform

  ~

the Mul1uFlags and Mul1u instructions perform

    the 64b multiplication using only 64b registers, however the
    method used causes the high 64b to be corrupted for certain
    inputs. here we fix the computation.

Diff:

Revision 4 (+11 -9)

Show changes

Ship it!
Posted (Feb. 4, 2017, 8:21 a.m.)

Lol, that code is hard to understand. But, LGTM.

What are you using to test this? Any chance you can commit the test so we don't accidentally break this again in the future?

  1. This bug was manifesting in the ROCr runtime while it is loading libraries, it manifested as a segfualt (unmapped addr panic) because an address calculation was corrupted due to this instruction. I manually tested this instruction by comparing its output to the result of 64b multiplication using __uint128_t. This is a perfect example of where instruction tests would be useful. It only seemed to give bad output with certain inputs, which gave the error the appearance of being non-deterministic.

  2. Resurecting this from the grave... does anyone have any objections to me just pushing this? I spent all day tracking down a bug, which was fixed by this patch.

  3. This is an important patch for x86, I say push it. Also it would be nice if we were eventually able to use the CheckerCpu for x86 (I believe it's ARM-only right now), as there might be more hard-to-find bugs like this one.