We miss MacroAssemblerMIPS::or32(TrustedImm32, Address).
Created attachment 304573 [details] Patch The patch.
Comment on attachment 304573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304573&action=review Reviewing informally: The comparisons should treat immediate value zero as a 16-bit value which gets embedded in an “ori” instruction. Other than that looks good overall. There is the thing that I would prefer using constants from “stdint.h” or masking with 0xFFFF to check for values outside of the 16-bit range, but the rest of the code in the file compares directly against the numeric values, so I don't have a strong feeling about that. It might be a good idea to do change the code to use [U]INTx_{MIN,MAX} or bitmaks, as a separate patch. WDYT? > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:448 > + if (address.offset >= -32768 && address.offset <= 32767 Personally, I would write this condition as... if (address.offset & 0xFFFF == 0 && ...) ...because when reading this form it makes me think “this checks whether the upper 16-bits are unset”. On the other hand, comparisons against the integral values keep me wondering “wait, why is there a magic number here?”. I have no strong opinion about whether to use the bitmask version or the integer comparison, but if you would rather keep the integer comparison, please use INT16_MIN/INT16_MAX, which makes it more explicit that values outside the 16-bit integer range are being handled: if (address.offset >= INT16_MIN && address.offset <= INT16_MAX && ...) > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:457 > + if (imm.m_value > 0 && imm.m_value <= 65535 && !m_fixedWidth) This should be “greater or equal to zero”, not just ”greater than zero”: if (imm.m_value >= 0 && imm.m_value <= UINT16_MAX ...) or if (imm.m_value & 0xFFFF == 0 ...) If the immediate value is equal to zero, emitting the code for ORing could even be skipped altogether. Though I would do that separately on another patch. > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:477 > + if (imm.m_value > 0 && imm.m_value <= 65535 && !m_fixedWidth) Ditto: if (imm.m_value >= 0 && imm.m_value <= UINT16_MAX ...) ^^ greater or equal
(In reply to Adrian Perez from comment #2) > Comment on attachment 304573 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304573&action=review > > Reviewing informally: The comparisons should treat immediate value > zero as a 16-bit value which gets embedded in an “ori” instruction. > Other than that looks good overall. > > There is the thing that I would prefer using constants from “stdint.h” or > masking with 0xFFFF to check for values outside of the 16-bit range, but > the rest of the code in the file compares directly against the numeric > values, so I don't have a strong feeling about that. It might be a good > idea to do change the code to use [U]INTx_{MIN,MAX} or bitmaks, as a > separate patch. WDYT? > > > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:448 > > + if (address.offset >= -32768 && address.offset <= 32767 > > Personally, I would write this condition as... > > if (address.offset & 0xFFFF == 0 && ...) > > ...because when reading this form it makes me think “this checks whether > the upper 16-bits are unset”. On the other hand, comparisons against the > integral values keep me wondering “wait, why is there a magic number here?”. > > I have no strong opinion about whether to use the bitmask version or the > integer comparison, but if you would rather keep the integer comparison, > please use INT16_MIN/INT16_MAX, which makes it more explicit that values > outside the 16-bit integer range are being handled: > > if (address.offset >= INT16_MIN && address.offset <= INT16_MAX && ...) > Indeed, this would be clearer, especially with the bitmask I'd think. I'll create a separate bug and try to address that for the whole file. > > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:457 > > + if (imm.m_value > 0 && imm.m_value <= 65535 && !m_fixedWidth) > > This should be “greater or equal to zero”, not just ”greater than zero”: > > if (imm.m_value >= 0 && imm.m_value <= UINT16_MAX ...) > or if (imm.m_value & 0xFFFF == 0 ...) > > If the immediate value is equal to zero, emitting the code for ORing > could even be skipped altogether. Though I would do that separately > on another patch. I am cooking a patch that address this issues. Here again, I just copy-pasted from another version of the function, and that issue is a bit everywhere in the file and deserves its own bug I guess. > > > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:477 > > + if (imm.m_value > 0 && imm.m_value <= 65535 && !m_fixedWidth) > > Ditto: if (imm.m_value >= 0 && imm.m_value <= UINT16_MAX ...) > ^^ > greater or equal
Comment on attachment 304573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304573&action=review > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:481 > + m_assembler.addu(dataTempRegister, dataTempRegister, immTempRegister); Wow! I did not pay attention when doing that. This obviously should be an orInsn not an addu! Will also fix in the new version. The fact that this mistake was not obvious when looking at test results makes me think that this code path (or the whole function) might never be used.
Created attachment 306436 [details] Patch Patch with comments addressed. I am currently testing it to check it does not break anything (tests take about 10h to run, and bot is testing another revision before).
(In reply to Guillaume Emont from comment #5) > Created attachment 306436 [details] > Patch > > Patch with comments addressed. I am currently testing it to check it does > not break anything (tests take about 10h to run, and bot is testing another > revision before). And the test ran: this patch did not introduce any new crash, so I believe it's good to go.
\o/(In reply to Guillaume Emont from comment #6) > (In reply to Guillaume Emont from comment #5) > > Created attachment 306436 [details] > > Patch > > > > Patch with comments addressed. I am currently testing it to check it does > > not break anything (tests take about 10h to run, and bot is testing another > > revision before). > > And the test ran: this patch did not introduce any new crash, so I believe > it's good to go. \o/ Informal r+/cq+ from me.
Comment on attachment 306436 [details] Patch This is a rubber stamp since I don't understand any of it, but if Adrian says it's OK then let's do it.
Comment on attachment 306436 [details] Patch Clearing flags on attachment: 306436 Committed r215237: <http://trac.webkit.org/changeset/215237>
All reviewed patches have been landed. Closing bug.