I'm a bit suspicious of this new version and how it has fixed width masks and extra terms in the addition at the end. Could you please give me an example of where the original code got the wrong answer?
Gabe On Mon, May 15, 2017 at 1:09 PM, Jason Lowe-Power <[email protected]> wrote: > We should definitely look into the statetrace tool. I'll put it on my > giant gem5 list of things to do. > > Those of you in industry: There must be some test harnesses for hardware > and internal simulators. How do you test to make sure instructions are > implemented correctly? > > We've thought about using llvm or gcc tests, but they don't really test > what we need, IIRC. > > Maybe this would be a good summer project for an intern: hint, hint. > > Jason > > On Mon, May 15, 2017 at 3:04 PM Gross, Joe <[email protected]> wrote: > >> Probably we all agree that x86 could use some automated error checking, >> but I’m not sure of any way to do this without a ton of work. CheckerCPU >> seemed good, but as Gabe points out, this may not be comparing against the >> host hardware. >> >> >> >> Jason, do you know any way that’s partially or mostly implemented that we >> could reuse? >> >> >> >> Joe >> >> >> >> *From:* Jason Lowe-Power [mailto:[email protected]] >> *Sent:* Monday, May 15, 2017 2:54 PM >> *To:* Gabe Black <[email protected]>; gem5 Developer List < >> [email protected]> >> *Cc:* Gutierrez, Anthony <[email protected]>; Gross, Joe < >> [email protected]> >> *Subject:* Re: [gem5-dev] Review Request 3800: x86: fix Mul1u instruction >> >> >> >> 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
