Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2027773002:
Subzero, MIPS32: Handling floating point instructions fadd, fsub, fmul, fdiv (Closed)
|
Created:
4 years, 6 months ago by obucinac Modified:
4 years, 5 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionSubzero, MIPS32: Handling floating point instructions fadd, fsub, fmul, fdiv
This patch adds handling of floating point instructions
fadd, fsub, fmul and fdiv. Regarding frem, Mips32 does not have
instruction that calculates partial reminder, so it has to be
emulated with a set of instructions. Emulating frem will be addressed
in separate patch, when floating point format conversion instructions
are fully implemented.
BUG=
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=ab6a04f61a8d85a66524f36b6f6aee447f3dce4a
Patch Set 1 #
Total comments: 7
Patch Set 2 : Changes according to comments #Patch Set 3 : Added partial prerequisites #Patch Set 4 : Moving function return values to propper register type #Patch Set 5 : Rebase for applying prerequisites #
Total comments: 8
Patch Set 6 : RC changes #Messages
Total messages: 15 (3 generated)
Description was changed from ========== Subzero, MIPS32: Handling floating point instructions fadd, fsub, fmul, fdiv This patch adds handling of floating point instructions fadd, fsub, fmul and fdiv. Regarding frem, Mips32 does not have instruction that calculates partial reminder, so it has to be emulated with a set of instructions. Emulating frem will be addressed in separate patch, when floating point format conversion instructions are fully implemented. BUG= ========== to ========== Subzero, MIPS32: Handling floating point instructions fadd, fsub, fmul, fdiv This patch adds handling of floating point instructions fadd, fsub, fmul and fdiv. Regarding frem, Mips32 does not have instruction that calculates partial reminder, so it has to be emulated with a set of instructions. Emulating frem will be addressed in separate patch, when floating point format conversion instructions are fully implemented. BUG= ==========
https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:413: UnimplementedError(getFlags()); When I run the updated lit test, I'm getting a failure due to liveness validation failure. I suspect this part of the code is responsible, because this is the first unimplemented error seen when I remove the "--skip-unimplemented" flag. https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:820: case InstArithmetic::Frem: For frem, all the other targets create a call to the frem_f32 or frem_f64 runtime helper, you may want to take this same approach to keep things simple. In practice, there don't seem to be a lot of frem calls so there's not a performance issue. https://codereview.chromium.org/2027773002/diff/1/tests_lit/llvm2ice_tests/fp... File tests_lit/llvm2ice_tests/fp.arith.ll (right): https://codereview.chromium.org/2027773002/diff/1/tests_lit/llvm2ice_tests/fp... tests_lit/llvm2ice_tests/fp.arith.ll:25: ; RUN: --command %p2i --filetype=asm --assemble --disassemble --target mips32 \ 80-col
https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:413: UnimplementedError(getFlags()); On 2016/06/01 13:52:13, stichnot wrote: > When I run the updated lit test, I'm getting a failure due to liveness > validation failure. I suspect this part of the code is responsible, because > this is the first unimplemented error seen when I remove the > "--skip-unimplemented" flag. This is because calling convention is still not in place, and handling fp arguments (loading it to fp register) is missing. I believe this is dependency: https://codereview.chromium.org/2021033002/ https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:820: case InstArithmetic::Frem: On 2016/06/01 13:52:13, stichnot wrote: > For frem, all the other targets create a call to the frem_f32 or frem_f64 > runtime helper, you may want to take this same approach to keep things simple. > In practice, there don't seem to be a lot of frem calls so there's not a > performance issue. Acknowledged. https://codereview.chromium.org/2027773002/diff/1/tests_lit/llvm2ice_tests/fp... File tests_lit/llvm2ice_tests/fp.arith.ll (right): https://codereview.chromium.org/2027773002/diff/1/tests_lit/llvm2ice_tests/fp... tests_lit/llvm2ice_tests/fp.arith.ll:25: ; RUN: --command %p2i --filetype=asm --assemble --disassemble --target mips32 \ On 2016/06/01 13:52:13, stichnot wrote: > 80-col Done.
https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:413: UnimplementedError(getFlags()); On 2016/06/01 14:21:16, obucinac wrote: > On 2016/06/01 13:52:13, stichnot wrote: > > When I run the updated lit test, I'm getting a failure due to liveness > > validation failure. I suspect this part of the code is responsible, because > > this is the first unimplemented error seen when I remove the > > "--skip-unimplemented" flag. > > This is because calling convention is still not in place, and handling fp > arguments (loading it to fp register) is missing. > > I believe this is dependency: > https://codereview.chromium.org/2021033002/ > Hmm, I get the same unimplemented error in this place, even with that patch applied on top of this one. I looked into some options for creating float/double operands that could be used instead of function arguments: - %add = fadd float undef, undef - %a = load float, float* %addr, align 4 - cast/convert something to float Unfortunately, in all these cases it looks like we're still missing a bit of functionality elsewhere. We can't really check in code that breaks the tests like this CL currently does. I think the CL is otherwise good, and the tests are good, so you could consider temporarily disabling the tests. My favorite way of temporarily disabling a lit test is to change "RUN:" to "RUIN:" .
On 2016/06/01 21:22:08, stichnot wrote: > https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... > File src/IceTargetLoweringMIPS32.cpp (right): > > https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... > src/IceTargetLoweringMIPS32.cpp:413: UnimplementedError(getFlags()); > On 2016/06/01 14:21:16, obucinac wrote: > > On 2016/06/01 13:52:13, stichnot wrote: > > > When I run the updated lit test, I'm getting a failure due to liveness > > > validation failure. I suspect this part of the code is responsible, because > > > this is the first unimplemented error seen when I remove the > > > "--skip-unimplemented" flag. > > > > This is because calling convention is still not in place, and handling fp > > arguments (loading it to fp register) is missing. > > > > I believe this is dependency: > > https://codereview.chromium.org/2021033002/ > > > > Hmm, I get the same unimplemented error in this place, even with that patch > applied on top of this one. > > I looked into some options for creating float/double operands that could be used > instead of function arguments: > - %add = fadd float undef, undef > - %a = load float, float* %addr, align 4 > - cast/convert something to float > > Unfortunately, in all these cases it looks like we're still missing a bit of > functionality elsewhere. > > We can't really check in code that breaks the tests like this CL currently does. > I think the CL is otherwise good, and the tests are good, so you could consider > temporarily disabling the tests. > > My favorite way of temporarily disabling a lit test is to change "RUN:" to > "RUIN:" . Patch I referred to currently does not cover fp arguments, so lowerArguments does not lower floats, calling convention does not put them in place where they should be, and proper target register is not reserved. You are crashing at lowerArguments. Once all this is in place, this will run as expected. I can disable test or we can just wait for prerequisites to come, and then commit them. Disabling tests may come as forgetting to enable them later. Personally, I am for second approach, because patch is simple enough, and it is unlikely that some change will affect it signifficantly.
On 2016/06/02 11:42:02, obucinac wrote: > On 2016/06/01 21:22:08, stichnot wrote: > > > https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... > > File src/IceTargetLoweringMIPS32.cpp (right): > > > > > https://codereview.chromium.org/2027773002/diff/1/src/IceTargetLoweringMIPS32... > > src/IceTargetLoweringMIPS32.cpp:413: UnimplementedError(getFlags()); > > On 2016/06/01 14:21:16, obucinac wrote: > > > On 2016/06/01 13:52:13, stichnot wrote: > > > > When I run the updated lit test, I'm getting a failure due to liveness > > > > validation failure. I suspect this part of the code is responsible, > because > > > > this is the first unimplemented error seen when I remove the > > > > "--skip-unimplemented" flag. > > > > > > This is because calling convention is still not in place, and handling fp > > > arguments (loading it to fp register) is missing. > > > > > > I believe this is dependency: > > > https://codereview.chromium.org/2021033002/ > > > > > > > Hmm, I get the same unimplemented error in this place, even with that patch > > applied on top of this one. > > > > I looked into some options for creating float/double operands that could be > used > > instead of function arguments: > > - %add = fadd float undef, undef > > - %a = load float, float* %addr, align 4 > > - cast/convert something to float > > > > Unfortunately, in all these cases it looks like we're still missing a bit of > > functionality elsewhere. > > > > We can't really check in code that breaks the tests like this CL currently > does. > > I think the CL is otherwise good, and the tests are good, so you could > consider > > temporarily disabling the tests. > > > > My favorite way of temporarily disabling a lit test is to change "RUN:" to > > "RUIN:" . > > > Patch I referred to currently does not cover fp arguments, so lowerArguments > does not lower floats, calling convention does not put them in place where they > should be, and proper target register is not reserved. You are crashing at > lowerArguments. Once all this is in place, this will run as expected. > > I can disable test or we can just wait for prerequisites to come, and then > commit them. Disabling tests may come as forgetting to enable them later. > > Personally, I am for second approach, because patch is simple enough, and it is > unlikely that some change will affect it signifficantly. OK, waiting for prerequisites to land sounds fine. Please send a ping on this CL when it's ready.
On 2016/06/02 13:54:28, stichnot wrote: > OK, waiting for prerequisites to land sounds fine. Please send a ping on this > CL when it's ready. It appears prerequisites are in place now.
On 2016/06/13 14:03:22, obucinac wrote: > On 2016/06/02 13:54:28, stichnot wrote: > > OK, waiting for prerequisites to land sounds fine. Please send a ping on this > > CL when it's ready. > > It appears prerequisites are in place now. The rebase isn't quite working for me, maybe because of some very recent commits. I think I fixed the rebase errors locally, but I'm not allowed to upload a patchset to a CL I don't own. The UnimplementedErrors seem to be gone, but there are some liveness validation errors when running check-lit. I should be able to find some of the problems and I'll comment on the source file separately.
https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:462: // UnimplementedError(getFlags()); remove this https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1209: _mov_s(Dest, T); From this, I am seeing invalid asm code being generated: <stdin>:9:5: error: invalid operand for instruction sw $f12, 8($sp) ^ I think maybe swc1 should be emitted instead? https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1255: case InstArithmetic::Frem: For now, avoid a liveness validation error by adding FakeUses of the temporaries: Context.insert<InstFakeUse>(Src0R); Context.insert<InstFakeUse>(Src1R); The Om1 register allocator doesn't like it when infinite-weight temporaries have a definition but no uses. (Technically that's not really an error, but it may reflect what may be "sloppy" lowering so we decided to retain that error reporting.) https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1385: // if (Instr->getNumArgs()) { remove this https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1445: Context.insert<InstFakeDef>(ReturnReg); InstFakeUse https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1900: UnimplementedLoweringError(this, Instr); You probably want a "break;" after this, otherwise it will fallthrough to the f32 case when running with -skip-unimplemented. https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2049: // UnimplementedError(getFlags()); remove this https://codereview.chromium.org/2027773002/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:2055: // UnimplementedError(getFlags()); remove this
Ping...
lgtm
Description was changed from ========== Subzero, MIPS32: Handling floating point instructions fadd, fsub, fmul, fdiv This patch adds handling of floating point instructions fadd, fsub, fmul and fdiv. Regarding frem, Mips32 does not have instruction that calculates partial reminder, so it has to be emulated with a set of instructions. Emulating frem will be addressed in separate patch, when floating point format conversion instructions are fully implemented. BUG= ========== to ========== Subzero, MIPS32: Handling floating point instructions fadd, fsub, fmul, fdiv This patch adds handling of floating point instructions fadd, fsub, fmul and fdiv. Regarding frem, Mips32 does not have instruction that calculates partial reminder, so it has to be emulated with a set of instructions. Emulating frem will be addressed in separate patch, when floating point format conversion instructions are fully implemented. BUG= R=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 #6 (id:100001) manually as ab6a04f61a8d85a66524f36b6f6aee447f3dce4a (presubmit successful). |
