Yeah, please wait, and also please put it on gerrit. I want to understand why the original code was wrong and how the new code fixes it. The reason that bit of code is fairly complicated is that it's trying to efficiently do an n bit X n bit => 2n bit multiplication where the 2n bit product is split across two registers. I'll have to work through it again to remind myself how it was supposed to work.
Gabe On Mon, May 15, 2017 at 12:53 PM, Jason Lowe-Power <[email protected]> wrote: > Gabe: Do you want me to wait to push this patch? We can have Tony re-post > to gerrit if you want to review it. That would be way easier for me anyway. > > We need a way to test for these kinds of errors. It's quite upsetting that > we've had this error in multiply for so long. > > Jason > > On Mon, May 15, 2017 at 2:50 PM Gabe Black <[email protected]> wrote: > >> Please give me a chance to look at it. I think the checker CPU is for >> comparing, say, the O3 CPU against what's effectively the simple CPU. If an >> instruction doesn't do the right thing, it will do the wrong thing no >> matter which CPU is executing it. You can compare against actual execution >> on a real CPU using the statetrace tool, although I saw that somebody >> rearranged all the process startup code which I'd originally carefully >> matched against actual Linux process startup, so that tool probably doesn't >> give any useful information any more. >> >> Gabe >> >> On Mon, May 15, 2017 at 12:45 PM, Joe Gross <[email protected]> wrote: >> >>> >>> >>> > On Feb. 4, 2017, 10:21 a.m., Jason Lowe-Power wrote: >>> > > 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? >>> > >>> > Tony Gutierrez wrote: >>> > 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. >>> > >>> > Jason Lowe-Power wrote: >>> > 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. >>> >>> 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. >>> >>> >>> - Joe >>> >> >>> >>> ----------------------------------------------------------- >> >> >>> This is an automatically generated e-mail. To reply, visit: >>> >> http://reviews.gem5.org/r/3800/#review9400 >>> ----------------------------------------------------------- >>> >>> >>> On Feb. 3, 2017, 6:13 p.m., Tony Gutierrez wrote: >>> > >>> > ----------------------------------------------------------- >> >> >>> > This is an automatically generated e-mail. To reply, visit: >>> > http://reviews.gem5.org/r/3800/ >>> >> > ----------------------------------------------------------- >>> > >>> > (Updated Feb. 3, 2017, 6:13 p.m.) >>> > >>> > >>> > Review request for Default. >>> > >>> > >>> > Repository: gem5 >>> > >>> > >>> > Description >>> > ------- >>> >> >>> > >>> > 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. >>> > >>> > >>> > Diffs >>> > ----- >>> > >>> > src/arch/x86/isa/microops/regop.isa ed89cb178ecd7586296d2a2e835951 >>> 74474db554 >>> > >>> > Diff: http://reviews.gem5.org/r/3800/diff/ >>> > >>> > >>> > Testing >>> > ------- >>> > >>> > >>> > Thanks, >>> > >>> > Tony Gutierrez >>> > >>> > >>> >>> _______________________________________________ >>> gem5-dev mailing list >>> [email protected] >>> http://m5sim.org/mailman/listinfo/gem5-dev >>> >> _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
