I posted the change on Gerrit here: https://gem5-review.googlesource.com/c/3400/; feel free to continue the discussion there.
It’s been a while since I posted the original patch, so I don’t remember precisely which inputs stressed this bug. Basically some precision was lost in the upper bits of one of the partial products due to overflow. The computation was part of some address calculation that eventually led to a segfault in the application under test because the address ends up pointing to some random memory. This patch fixed that issue, as well as Jason’s it seems, among a few other issues that mostly manifested as bogus application output (i.e., silent data corruption in the app under test’s results). From: Gabe Black [mailto:[email protected]] Sent: Monday, May 15, 2017 2:44 PM To: Jason Lowe-Power <[email protected]> Cc: Gross, Joe <[email protected]>; gem5 Developer List <[email protected]>; Gutierrez, Anthony <[email protected]> Subject: Re: [gem5-dev] Review Request 3800: x86: fix Mul1u instruction Could you give me the specific numbers that are multiplied that don't work? Gabe On Mon, May 15, 2017 at 2:42 PM, Jason Lowe-Power <[email protected]<mailto:[email protected]>> wrote: Something that's really easy to test is printf("%lu\n", (unsigned long)-1); This is what I ran into today. This gives an unmapped memory error because of an incorrect multiply (which is how itoa is implemented). This example works with the patch provided. I think you could come up with many other examples. I think it's the upper-order bit calculation that's wrong. So unsigned multiplies near 2^64 give wrong answers. Other examples are hard to write since printf causes a crash. Jason On Mon, May 15, 2017 at 4:38 PM Gabe Black <[email protected]<mailto:[email protected]>> wrote: 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]<mailto:[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]<mailto:[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]<mailto:[email protected]>] Sent: Monday, May 15, 2017 2:54 PM To: Gabe Black <[email protected]<mailto:[email protected]>>; gem5 Developer List <[email protected]<mailto:[email protected]>> Cc: Gutierrez, Anthony <[email protected]<mailto:[email protected]>>; Gross, Joe <[email protected]<mailto:[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]<mailto:[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]<mailto:[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 ed89cb178ecd7586296d2a2e83595174474db554 > > Diff: http://reviews.gem5.org/r/3800/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected]<mailto:[email protected]> http://m5sim.org/mailman/listinfo/gem5-dev _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
