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 <gabebl...@google.com> 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 <joseph.gr...@amd.com> 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 >> ed89cb178ecd7586296d2a2e83595174474db554 >> > >> > Diff: http://reviews.gem5.org/r/3800/diff/ >> > >> > >> > Testing >> > ------- >> > >> > >> > Thanks, >> > >> > Tony Gutierrez >> > >> > >> >> _______________________________________________ >> gem5-dev mailing list >> gem5-dev@gem5.org >> http://m5sim.org/mailman/listinfo/gem5-dev >> > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
