Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1716483003:
Subzero: implement 64 bit multiply in mips32 (Closed)
|
Created:
4 years, 10 months ago by rkotlerimgtec Modified:
4 years, 10 months ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionSubzero: implement 64 bit multiply in mips32
Implement 64 bit multiply in mips32 and, in addition, add the lo/hi registers which are also used for other 64 bit math such as div, rem.
BUG=
R=jpp@chromium.org, stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=a80cdbc29085d2d8582a54441f41bb444bf31dca
Patch Set 1 #
Total comments: 8
Patch Set 2 : changes suggested by stichnot #
Total comments: 2
Patch Set 3 : changes suggested by stichnot #
Messages
Total messages: 20 (6 generated)
Description was changed from ========== implement 64 bit multiply in mips32 BUG= ========== to ========== implement 64 bit multiply in mips32 and in addition add the lo/hi registers which are also used for other 64 bit math such as div, rem. BUG= ==========
rkotlerimgtec@gmail.com changed reviewers: + jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org
lgtm https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:657: _mul(TM1, Src0HiR, Src1LoR); I'm late to the party, but what's the difference between mult and mul?
mul is a pure signed 32 bit operation, i.e. no overflow, 32 bit multiply
with 32 bit result
mult/ multu do a 32 bit multiply (signed/unsigned) with a 64 bit result in
LO, HI
those are special registers .. accumulators, you can only address LO, HI
with these special
M{T=to,F=from} {LO,HI} instructions
in mipsr6 which is new, we have done away with LO, HI
i'm doing mips32r5 port first but will later support all the various
mips32/64 r5/6 and micromips
(like ARM thumb2) variants. most likely subzero will never support mips16
(like ARM thumb1)
LO,HI are unpredicatable after mul and I need to really account for that
but have not done that yet.
There are a bunch of complications with this and I need to think it through
and figure out the best place
to put this in subzero.
On Thu, Feb 18, 2016 at 6:52 PM, <jpp@chromium.org> wrote:
> lgtm
>
>
>
>
https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32...
> File src/IceTargetLoweringMIPS32.cpp (right):
>
https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32...
> src/IceTargetLoweringMIPS32.cpp:657
<https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32...>:
_mul(TM1, Src0HiR, Src1LoR);
> I'm late to the party, but what's the difference between mult and mul?
> https://codereview.chromium.org/1716483003/
>
>
--
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.
Now that I have the LO, HI registers I could just do a fakedef after mul of LO, HI. On Thu, Feb 18, 2016 at 7:06 PM, reed kotler <rkotlerimgtec@gmail.com> wrote: > mul is a pure signed 32 bit operation, i.e. no overflow, 32 bit multiply > with 32 bit result > > mult/ multu do a 32 bit multiply (signed/unsigned) with a 64 bit result in > LO, HI > those are special registers .. accumulators, you can only address LO, HI > with these special > M{T=to,F=from} {LO,HI} instructions > > in mipsr6 which is new, we have done away with LO, HI > > i'm doing mips32r5 port first but will later support all the various > mips32/64 r5/6 and micromips > (like ARM thumb2) variants. most likely subzero will never support mips16 > (like ARM thumb1) > > LO,HI are unpredicatable after mul and I need to really account for that > but have not done that yet. > There are a bunch of complications with this and I need to think it > through and figure out the best place > to put this in subzero. > > > > > > On Thu, Feb 18, 2016 at 6:52 PM, <jpp@chromium.org> wrote: > >> lgtm >> >> >> >> https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... >> File src/IceTargetLoweringMIPS32.cpp (right): >> https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... >> src/IceTargetLoweringMIPS32.cpp:657 <https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32...>: _mul(TM1, Src0HiR, Src1LoR); >> I'm late to the party, but what's the difference between mult and mul? >> https://codereview.chromium.org/1716483003/ >> >> > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
There are some other weird case of how LO, HI can interact with mul. Anyway, I need to think this through carefully and will do it later. Maybe there should be a TODO in the code here. On Thu, Feb 18, 2016 at 7:08 PM, reed kotler <rkotlerimgtec@gmail.com> wrote: > Now that I have the LO, HI registers I could just do a fakedef after mul > of LO, HI. > > > > On Thu, Feb 18, 2016 at 7:06 PM, reed kotler <rkotlerimgtec@gmail.com> > wrote: > >> mul is a pure signed 32 bit operation, i.e. no overflow, 32 bit multiply >> with 32 bit result >> >> mult/ multu do a 32 bit multiply (signed/unsigned) with a 64 bit result >> in LO, HI >> those are special registers .. accumulators, you can only address LO, HI >> with these special >> M{T=to,F=from} {LO,HI} instructions >> >> in mipsr6 which is new, we have done away with LO, HI >> >> i'm doing mips32r5 port first but will later support all the various >> mips32/64 r5/6 and micromips >> (like ARM thumb2) variants. most likely subzero will never support mips16 >> (like ARM thumb1) >> >> LO,HI are unpredicatable after mul and I need to really account for that >> but have not done that yet. >> There are a bunch of complications with this and I need to think it >> through and figure out the best place >> to put this in subzero. >> >> >> >> >> >> On Thu, Feb 18, 2016 at 6:52 PM, <jpp@chromium.org> wrote: >> >>> lgtm >>> >>> >>> >>> https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... >>> File src/IceTargetLoweringMIPS32.cpp (right): >>> https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... >>> src/IceTargetLoweringMIPS32.cpp:657 <https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32...>: _mul(TM1, Src0HiR, Src1LoR); >>> I'm late to the party, but what's the difference between mult and mul? >>> https://codereview.chromium.org/1716483003/ >>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
For example, (re. MTLO in instruction: "A computed result written to the HI/LO pair by DIV, DIVU, MULT, or MULTU must be read by MFHI or MFLO before a new result can be written into either HI or LO. If an MTLO instruction is executed following one of these arithmetic instructions, but before an MFLO or MFHI instruction, the contents of HI are UNPREDICTABLE. The following example shows this illegal situation: MULT ... MTLO ... MFHI r2,r4 # start operation that will eventually write to HI,LO # code not containing mfhi or mflo r6 # code not containing mfhi # this mfhi would get an UNPREDICTABLE value" On Thu, Feb 18, 2016 at 7:17 PM, reed kotler <rkotlerimgtec@gmail.com> wrote: > There are some other weird case of how LO, HI can interact with mul. > Anyway, I need to think this through carefully and will do it later. Maybe > there should be a TODO > in the code here. > > > > On Thu, Feb 18, 2016 at 7:08 PM, reed kotler <rkotlerimgtec@gmail.com> > wrote: > >> Now that I have the LO, HI registers I could just do a fakedef after mul >> of LO, HI. >> >> >> >> On Thu, Feb 18, 2016 at 7:06 PM, reed kotler <rkotlerimgtec@gmail.com> >> wrote: >> >>> mul is a pure signed 32 bit operation, i.e. no overflow, 32 bit multiply >>> with 32 bit result >>> >>> mult/ multu do a 32 bit multiply (signed/unsigned) with a 64 bit result >>> in LO, HI >>> those are special registers .. accumulators, you can only address LO, HI >>> with these special >>> M{T=to,F=from} {LO,HI} instructions >>> >>> in mipsr6 which is new, we have done away with LO, HI >>> >>> i'm doing mips32r5 port first but will later support all the various >>> mips32/64 r5/6 and micromips >>> (like ARM thumb2) variants. most likely subzero will never support >>> mips16 (like ARM thumb1) >>> >>> LO,HI are unpredicatable after mul and I need to really account for that >>> but have not done that yet. >>> There are a bunch of complications with this and I need to think it >>> through and figure out the best place >>> to put this in subzero. >>> >>> >>> >>> >>> >>> On Thu, Feb 18, 2016 at 6:52 PM, <jpp@chromium.org> wrote: >>> >>>> lgtm >>>> >>>> >>>> >>>> https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... >>>> File src/IceTargetLoweringMIPS32.cpp (right): >>>> https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... >>>> src/IceTargetLoweringMIPS32.cpp:657 <https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32...>: _mul(TM1, Src0HiR, Src1LoR); >>>> I'm late to the party, but what's the difference between mult and mul? >>>> https://codereview.chromium.org/1716483003/ >>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
I think that most of these special cases do not apply at all to mips R5 and above so it may not be an issue at all. (only to MIPS I-III) On Thu, Feb 18, 2016 at 7:19 PM, reed kotler <rkotlerimgtec@gmail.com> wrote: > For example, (re. MTLO in instruction: > > "A computed result written to the HI/LO pair by DIV, DIVU, MULT, or MULTU > must be read by MFHI or MFLO > before a new result can be written into either HI or LO. > If an MTLO instruction is executed following one of these arithmetic > instructions, but before an MFLO or MFHI > instruction, the contents of HI are UNPREDICTABLE. The following example > shows this illegal situation: > MULT > ... > MTLO > ... > MFHI > r2,r4 > # start operation that will eventually write to HI,LO > # code not containing mfhi or mflo > r6 > # code not containing mfhi > # this mfhi would get an UNPREDICTABLE value" > > > > On Thu, Feb 18, 2016 at 7:17 PM, reed kotler <rkotlerimgtec@gmail.com> > wrote: > >> There are some other weird case of how LO, HI can interact with mul. >> Anyway, I need to think this through carefully and will do it later. >> Maybe there should be a TODO >> in the code here. >> >> >> >> On Thu, Feb 18, 2016 at 7:08 PM, reed kotler <rkotlerimgtec@gmail.com> >> wrote: >> >>> Now that I have the LO, HI registers I could just do a fakedef after mul >>> of LO, HI. >>> >>> >>> >>> On Thu, Feb 18, 2016 at 7:06 PM, reed kotler <rkotlerimgtec@gmail.com> >>> wrote: >>> >>>> mul is a pure signed 32 bit operation, i.e. no overflow, 32 bit >>>> multiply with 32 bit result >>>> >>>> mult/ multu do a 32 bit multiply (signed/unsigned) with a 64 bit result >>>> in LO, HI >>>> those are special registers .. accumulators, you can only address LO, >>>> HI with these special >>>> M{T=to,F=from} {LO,HI} instructions >>>> >>>> in mipsr6 which is new, we have done away with LO, HI >>>> >>>> i'm doing mips32r5 port first but will later support all the various >>>> mips32/64 r5/6 and micromips >>>> (like ARM thumb2) variants. most likely subzero will never support >>>> mips16 (like ARM thumb1) >>>> >>>> LO,HI are unpredicatable after mul and I need to really account for >>>> that but have not done that yet. >>>> There are a bunch of complications with this and I need to think it >>>> through and figure out the best place >>>> to put this in subzero. >>>> >>>> >>>> >>>> >>>> >>>> On Thu, Feb 18, 2016 at 6:52 PM, <jpp@chromium.org> wrote: >>>> >>>>> lgtm >>>>> >>>>> >>>>> >>>>> https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... >>>>> File src/IceTargetLoweringMIPS32.cpp (right): >>>>> https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... >>>>> src/IceTargetLoweringMIPS32.cpp:657 <https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32...>: _mul(TM1, Src0HiR, Src1LoR); >>>>> I'm late to the party, but what's the difference between mult and mul? >>>>> https://codereview.chromium.org/1716483003/ >>>>> >>>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1716483003/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:657: _mul(TM1, Src0HiR, Src1LoR); On 2016/02/19 02:52:40, John wrote: > I'm late to the party, but what's the difference between mult and mul? mult is signed 64 bit result in Lo/Hi mul is 32 bit signed result
Sorry that I can't find any official documentation to reference, so I'll have to repeat myself here. The Subject line of this CL is great. Unfortunately the Subject field does not go into the git commit message. The git commit message is just the Description field, plus some auto-generated info like a URL for the CL. As such, you almost always want to duplicate the Subject line into the beginning of the Description. So this CL Description should look something like this: Subzero: implement 64 bit multiply in mips32 Implement 64 bit multiply in mips32 [...] https://codereview.chromium.org/1716483003/diff/1/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1716483003/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:106: X(Reg_LO, = Reg_ZERO + 32, "lo", 0, 0, 0, 0, 1, 0, 0, 0, 0, \ You have put 1's in the isInt column. This makes it available to the register allocator for normal integer variables, which is probably not what you want. Most likely these attributes should all be 0, like with the "zero" register. https://codereview.chromium.org/1716483003/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:106: X(Reg_LO, = Reg_ZERO + 32, "lo", 0, 0, 0, 0, 1, 0, 0, 0, 0, \ The "encode" column should probably have some sentinel value like 0, since I'm sure the values 32 and 33 don't actually appear as a 6-bit sequence in encoded instructions that use these registers. Also - for a separate CL - the values in the "encode" column should be constants like 0,1,2,3,... and definitely not constexprs that depend on the RegNumT enum value. https://codereview.chromium.org/1716483003/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:149: X(Reg_LOHI, 8, "lo, hi", 1, 0, 0, 0, 0, 1, 0, 0, 0, \ Same comments as above - 0's in all the bool columns, and a sentinel value like 0 for the encoded value instead of 8.
Description was changed from ========== implement 64 bit multiply in mips32 and in addition add the lo/hi registers which are also used for other 64 bit math such as div, rem. BUG= ========== to ========== Subzero: implement 64 bit multiply in mips32 Implement 64 bit multiply in mips32 and, in addition, add the lo/hi registers which are also used for other 64 bit math such as div, rem. BUG= ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1716483003/diff/1/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1716483003/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:106: X(Reg_LO, = Reg_ZERO + 32, "lo", 0, 0, 0, 0, 1, 0, 0, 0, 0, \ On 2016/02/19 14:53:09, stichnot wrote: > The "encode" column should probably have some sentinel value like 0, since I'm > sure the values 32 and 33 don't actually appear as a 6-bit sequence in encoded > instructions that use these registers. > > Also - for a separate CL - the values in the "encode" column should be constants > like 0,1,2,3,... and definitely not constexprs that depend on the RegNumT enum > value. Done. https://codereview.chromium.org/1716483003/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:106: X(Reg_LO, = Reg_ZERO + 32, "lo", 0, 0, 0, 0, 1, 0, 0, 0, 0, \ On 2016/02/19 14:53:09, stichnot wrote: > You have put 1's in the isInt column. This makes it available to the register > allocator for normal integer variables, which is probably not what you want. > Most likely these attributes should all be 0, like with the "zero" register. Done. https://codereview.chromium.org/1716483003/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:149: X(Reg_LOHI, 8, "lo, hi", 1, 0, 0, 0, 0, 1, 0, 0, 0, \ On 2016/02/19 14:53:09, stichnot wrote: > Same comments as above - 0's in all the bool columns, and a sentinel value like > 0 for the encoded value instead of 8. Done.
https://codereview.chromium.org/1716483003/diff/60001/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1716483003/diff/60001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:44: X(Reg_AT, =1, "at", 0, 0, 0, 0, 0, 0, 0, 0, 0, \ As long as you're making the change to remove Reg_ZERO from the encode column, can you also remove the '='? You would have to add the '=' to the x-macro in IceRegistersMIPS32.h, but it will be a lot cleaner that way.
https://codereview.chromium.org/1716483003/diff/60001/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1716483003/diff/60001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:44: X(Reg_AT, =1, "at", 0, 0, 0, 0, 0, 0, 0, 0, 0, \ On 2016/02/20 00:03:59, stichnot wrote: > As long as you're making the change to remove Reg_ZERO from the encode column, > can you also remove the '='? You would have to add the '=' to the x-macro in > IceRegistersMIPS32.h, but it will be a lot cleaner that way. Done.
lgtm
Description was changed from ========== Subzero: implement 64 bit multiply in mips32 Implement 64 bit multiply in mips32 and, in addition, add the lo/hi registers which are also used for other 64 bit math such as div, rem. BUG= ========== to ========== Subzero: implement 64 bit multiply in mips32 Implement 64 bit multiply in mips32 and, in addition, add the lo/hi registers which are also used for other 64 bit math such as div, rem. BUG= R=jpp@chromium.org, stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) manually as a80cdbc29085d2d8582a54441f41bb444bf31dca (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
