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]> 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]> 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]> >> 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
