[cfe-commits] [llvm-commits] Fix bug 12212 Missing mul-to-shift conversion for negative non-power-of 2 constants on ARM

Weiming Zhao weimingz at codeaurora.org
Fri Mar 9 13:30:56 PST 2012


Hi Anton,

Thanks for the careful review and the feedback.

I'm attaching an updated patch for the fix per your suggestion:
1. removed the new line before  "{"
2. moved the MulAmtAbs to the path when MulAmt<0

I run the test suite again. No bugs.

The patch for the new test cases is the same. (I'm attaching it again)

Please feel free to let me know if there is any other issues.

Thanks,
Weiming


-----Original Message-----
From: Anton Korobeynikov [mailto:anton at korobeynikov.info] 
Sent: Friday, March 09, 2012 2:20 AM
To: Weiming Zhao
Cc: cfe-commits at cs.uiuc.edu; llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] Fix bug 12212 Missing mul-to-shift conversion for negative non-power-of 2 constants on ARM

Hello

> Please review it.
Generally looks good. Two nit-picks:
1. Make sure your code style matches the style of surrounding code (here, in particular - the newlines before / after "{" and "}") 2. I don't think you need separate MulAmtAbs variable - after all, later you have if() basing on the sign of MulAmtAbs, so, you can just fold the proper absolute value inside the branches of if()

Thanks for working on this!
--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Bug12212-LLVM-misses-multiply-by-non-power-of-2-negat.patch
Type: application/octet-stream
Size: 3276 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120309/3ebf6da7/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-update-unit-test.patch
Type: application/octet-stream
Size: 1413 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120309/3ebf6da7/attachment-0001.obj>


More information about the cfe-commits mailing list