[LLVMdev] review request for patch
parseerror at gmail.com
Sun Jul 12 08:59:15 PDT 2009
On Sat, Jul 11, 2009 at 5:06 PM, Nick Lewycky<nicholas at mxc.ca> wrote:
> Hi Ryan,
> In this case there already is an implementation for this, it's just hard
> to find being in the internals of the LoopVR pass. I'm planning to pull
> the multiply and udiv support out of there.
> Your patch looks good but beyond what Dan mentioned you have a bug
> calculating NewUpper: the constant ranges are half-open intervals where
> "[5, 10)" includes the value 9 but not 10. This means you need to
> subtract 1 before multiplying the uppers then add one on when you're
> done otherwise you end up with [5, 11) * [1, 3) = [5, 33) when it should
> equal [5, 21).
> Ryan Flynn wrote:
>> On Fri, Jul 10, 2009 at 8:35 PM, Dan Gohman<gohman at apple.com> wrote:
>>> On Jul 10, 2009, at 2:03 PM, Ryan Flynn <parseerror at gmail.com> wrote:
>>>> I've addressed a "TODO" in ConstantRange and several in its unit test
>>>> by implementing a stricter "multiply" method (it had been returning a
>>>> "full" set for anything that wasn't "empty", which broader than
>>>> and updated the unit test to match, but I'm not completely confident
>>>> that I understand ConstantRange and APInt and was hoping someone more
>>>> familiar might take a quick look and give me some feedback.
>>>> Thanks in advance.
>>> Thanks for working on this! Here are some comments:
>>> The overflow check isn't sufficient. Unlike add, multiply can wrap
>>> around multiple times, so it needs a more involved check. One way to
>>> do this would be to extend the operands out to twice their original
>>> width, do the multiply, and then check for overflow.
>>> For the unit tests, please check for specific values instead of
>>> checking that One.multiply(Wrap) is equal to Wrap.multply(One) for
>>> example, so that it won't report a pass if it both sides have the same
>>> wrong value.
>>>> P.S. - Also, I'm not totally sure if it's appropriate to ask for this
>>>> here, but I thought it was more so than to llvm-commit, please let me
>>>> know otherwise as I am new around here.
>>> llvm-commits is the usual place for patches, but this works too.
>> Thanks for the feedback, I will work on it.
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
Thank you for pointing out the bug, and seeing as how you're planning
to refactor existing stuff anyways i will leave this one to you.
More information about the llvm-dev