VRP for and,or,xor + Refactoring intrange.d + Fix Issue 10310 #7317

somzzz · 2017-11-14T16:49:21Z

As per discussions with @andralex @WalterBright and @tgehr , this PR is the first step to improving VRP in dmd. Reimplemented and, or, xor over IntRange using the algorithms designed by @tgehr :
https://github.com/tgehr/d-compiler/blob/master/vrange.d

Refactored intrange.d to use DMD2 style op overloads. Moved implementation for and, or, xor from dcast.d. This is a first step, the other operators will follow - this PR is quite big already.

Also fixing bug related to imprecise xor bounds:
https://issues.dlang.org/show_bug.cgi?id=10310

dlang-bot · 2017-11-14T16:49:22Z

Thanks for your pull request, @somzzz! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	10310	VRP for bitwise &

andralex

cc @tgehr @WalterBright

andralex · 2017-11-15T19:51:55Z

src/ddmd/intrange.d

@@ -77,35 +78,71 @@ struct SignExtendedNumber
            return 0;
    }

-    SignExtendedNumber opNeg() const
+    ref SignExtendedNumber opAddAssign(int a)


This is odd... why not opUnary(string op : "++")()?

andralex · 2017-11-15T19:53:30Z

src/ddmd/intrange.d

+
+    private alias SignExtendedNumber R;
+
+    R opUnary(string op:"~")() const


we use spaces around :, I recall Timon's spacing is more compact

andralex · 2017-11-15T20:49:32Z

src/ddmd/intrange.d

+    R opBinary(string op:"^^")(R rhs)
+    {
+        // Not yet implemented
+        assert(0);


Isn't this going to crash during use?

andralex · 2017-11-15T20:50:10Z

src/ddmd/intrange.d

+
+    private alias IntRange R;
+
+    R copy() const


Why is this necessary? Doesn't IntRange(another) suffice?

There isn't any IntRange constructor which takes another IntRange as param.

That would be implicitly defined, or you can define it if needed.

andralex · 2017-11-15T20:56:52Z

src/ddmd/intrange.d

+    R opUnary(string op:"-")()
+    {
+        // Not yet implemented
+        assert(0);


This looks fairly easy to implement.

andralex · 2017-11-15T20:58:48Z

src/ddmd/intrange.d

+        }
+
+        R l = this.copy();
+        R r = rhs.copy();


Aside from the copy brow-raiser, you may want to use lhs and rhs for the operands, e.g. in github's font l and 1 are difficult to distinguish.

andralex · 2017-11-15T21:03:37Z

src/ddmd/intrange.d

+    R opBinary(string op:"+")(R rhs)
+    {
+        // Not yet implemented
+        assert(0);


Reiterating suggestion: on overflow, conservatively return the maximum range.

This PR is only for the bitops, the old code doesn't use operator overloads for IntRange, so currently there is no code that calls into assert(0). Most of the implementation for these operators is split between dcast.d eg
https://github.com/dlang/dmd/blob/master/src/ddmd/dcast.d#L3613
and SignExtendedNumber and should be refactored. The purpose of adding those stubs is to suggest this.

I wanted to do this in different PRs. Similarly, at the end, if we decide we don't need SignExtendedNumber anymore, we can remove it in its own PR.

If you think it's better to handle all VRP in one go, we can do that.

andralex · 2017-11-15T21:04:49Z

src/ddmd/intrange.d

+private:
+    // Credits to Timon Gehr maxOr, minOr, maxAnd, minAnd
+    // https://github.com/tgehr/d-compiler/blob/master/vrange.d
+    static SignExtendedNumber maxOr(R lhs, R rhs){


Formatting issues - brace should be on its own line and the function body should be indented.

andralex · 2017-11-15T21:05:15Z

src/ddmd/intrange.d

+    auto xor = lhs.imax.value ^ rhs.imax.value;
+    auto and = lhs.imax.value & rhs.imax.value;
+
+    for(uinteger_t d = 1LU << (8 * uinteger_t.sizeof - 1); d; d >>= 1)


space after if etc (adjust from Timon's convention to ours)

andralex

no blockers

andralex · 2017-11-16T18:09:26Z

src/ddmd/intrange.d

@@ -77,35 +78,70 @@ struct SignExtendedNumber
            return 0;
    }

-    SignExtendedNumber opNeg() const
+    private alias SignExtendedNumber R;


This is a distraction, a future PR should eliminate it and rename SignExtendedNumber to SignExtended.

andralex · 2017-11-16T18:10:36Z

src/ddmd/intrange.d

+
+    R opUnary(string op : "~")() const
+    {
+        return SignExtendedNumber(~value);


I'm not sure this (and other similar operations) is correct. It converts a negative number into a positive one. Shouldn't it be return SignExtendedNumber(~value, negative);? cc @WalterBright @tgehr

~ flips the sign bit, so it might need to be SignExtendedNumber(~value, !negative) if the underlying datatype is signed.

@tgehr I actually tried that, then druntime failed to compile

andralex · 2017-11-16T18:25:07Z

src/ddmd/intrange.d

-        uinteger_t sum = value + a.value;
-        bool carry = sum < value && sum < a.value;
-        if (negative != a.negative)
+        return SignExtendedNumber(value & rhs.value);


same question here about signedness of result - looks like any two inputs produce a positive number

andralex · 2017-11-16T18:31:57Z

src/ddmd/intrange.d

+    R opBinary(string op : "+")(R rhs)
+    {
+        uinteger_t sum = value + rhs.value;
+        bool carry = sum < value && sum < rhs.value;


Clearer and more efficient:

bool carry = value > uinteger_t.max - rhs.value;

somzzz force-pushed the vrp_andOr branch from c484f27 to 530a700 Compare November 14, 2017 16:56

andralex requested a review from WalterBright November 15, 2017 19:47

andralex requested changes Nov 15, 2017

View reviewed changes

VRP and,or,xor + fix issue 10310

1757224

somzzz force-pushed the vrp_andOr branch from 530a700 to 1757224 Compare November 16, 2017 14:25

dlang-bot added the Bug Fix label Nov 16, 2017

andralex approved these changes Nov 16, 2017

View reviewed changes

andralex added the auto-merge label Nov 16, 2017

dlang-bot merged commit 6395d48 into dlang:master Nov 16, 2017

ibuclaw mentioned this pull request Dec 8, 2018

[dmd-cxx] Backport intrange patches and fix ubsan errors #9046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VRP for and,or,xor + Refactoring intrange.d + Fix Issue 10310 #7317

VRP for and,or,xor + Refactoring intrange.d + Fix Issue 10310 #7317

somzzz commented Nov 14, 2017

dlang-bot commented Nov 14, 2017 •

edited

andralex left a comment

andralex Nov 15, 2017

andralex Nov 15, 2017

andralex Nov 15, 2017

andralex Nov 15, 2017

somzzz Nov 16, 2017

andralex Nov 16, 2017

andralex Nov 15, 2017

andralex Nov 15, 2017

andralex Nov 15, 2017

somzzz Nov 16, 2017

andralex Nov 15, 2017

andralex Nov 15, 2017

andralex left a comment

andralex Nov 16, 2017

andralex Nov 16, 2017

tgehr Nov 17, 2017 •

edited

andralex Nov 17, 2017

andralex Nov 16, 2017

andralex Nov 16, 2017


		private alias SignExtendedNumber R;

		R opUnary(string op:"~")() const

VRP for and,or,xor + Refactoring intrange.d + Fix Issue 10310 #7317

VRP for and,or,xor + Refactoring intrange.d + Fix Issue 10310 #7317

Conversation

somzzz commented Nov 14, 2017

dlang-bot commented Nov 14, 2017 • edited

Bugzilla references

andralex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andralex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgehr Nov 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlang-bot commented Nov 14, 2017 •

edited

tgehr Nov 17, 2017 •

edited