Add VRP-based constant folding for integer comparisons. #5229

tsbockman · 2015-10-24T03:43:03Z

This is the first instalment of my translated and improved rendition of @lionello 's PR #1913, which I have decided to break up into chunks, since it makes several changes which do not need to be evaluated on an all-or-nothing basis.

This first piece:

Implements value-range-propagation based constant folding for integer comparisons.
Fixes issue 15585 by raising the initial VRP maximum for dchar values from dchar.max to uint.max.
Disables the statement is not reachable warning. That warning interacts very badly with effective VRP in generic code (DMD issue #14835), and when I made a forum thread to discuss the issue, the conclusion was that a true fix is not feasible.

Strategically, my goal is to enhance VRP and constant folding capabilities to the point that issue 259's pre-approved solution to the problem of dangerous signed/unsigned comparisons can be implemented without causing too many "false positive" errors.

This PR does not accomplish that goal by itself, but it is a useful step. In the mean time, it modestly improve the front-end's ability to optimize logic and detect unreachable code.

The change to dchar's initial VRP maximum is required to prevent the compiler from treating attempts to handle invalid out-of-range dchar values as dead code. I got @WalterBright 's approval for this part earlier on the forums: http://forum.dlang.org/post/oionrfexehapzicgpbrw@forum.dlang.org

JackStouffer · 2015-10-24T04:02:51Z

If this breaks Phobos, then that begs the question of what other code this could break in the wild. I think it would be prudent to at least try to compile some popular dub libraries with this just to be sure.

tsbockman · 2015-10-24T04:18:27Z

@JackStouffer Because of the dchar VRP change, this is basically guaranteed to break some other code in the wild. The existing behaviour is clearly wrong, though, and has the potential to cause nasty silent bugs.

With this PR, code which is potentially effected will mostly either:

Suddenly start working correctly, with no intervention required, or
Fail with a compiler message that basically says, "stick an explicit cast here to fix this".

Edit: I forgot that it turned out to be a cast(wchar) that was missing, not cast(dchar). So never mind about implicit uint to dchar conversions, at least for now.

This issue is only really likely to come up in unicode codecs; even then only one line in all of std.uni needed to be changed.

The other required Phobos change is potentially of greater concern, though, as it indicates that it is at least possible for integer comparison folding to interact with templates and dead code detection in awkward ways. So, I guess I can try compiling some other people's stuff to see how common that issue is.

tsbockman · 2015-10-24T05:45:35Z

@JackStouffer So I tried building some stuff from dub and learned:

Most stuff on dub fails to compile with DMD HEAD for reasons entirely unrelated to my PR, which makes it hard to test this way.
The dchar stuff is not a problem; no project I have tested besides Phobos has been effected, so far.
VRP integer comparison folding + templates + dead code detection thing is a problem, albeit almost exclusively on unittest builds, and then only for certain projects - most prominently, vibe-d.

Upon further investigation, I have determined that (3) is caused by the pre-existing DMD issue #14835. The problem is, the better that VRP and constant folding get, the worse that issue 14835 becomes.

lionello · 2015-10-24T09:06:35Z

I think the dchar.max patch should be filed separately. It is a bug and should get fixed asap. https://issues.dlang.org/show_bug.cgi?id=14627

tsbockman · 2015-10-24T16:03:36Z

@lionello This PR does not fix issue 14627. Your demo code still compiles and runs, even with this patch applied.

This PR does not change dchar.max or the dchar to uint conversion rules; it simply allows VRP to account for the possibility of out-of-range dchar values.

tsbockman · 2015-10-24T16:34:57Z

Since the companion Phobos PR #3767 has been merged now, I tweaked the comment on line 1007 of constfold.d to get the auto-tester to check this PR again.

WalterBright · 2015-10-25T19:20:30Z

src/constfold.d

+            n = e1.type.isunsigned() || e2.type.isunsigned() ?
+                intUnsignedCmp(op, n1, n2) :
+                intSignedCmp(op, n1, n2);
+        }
    }
    else


How about combining line 1002 with 1004 to make it else if? I think it would flow a little nicer.

tsbockman · 2015-10-26T16:29:41Z

@WalterBright Done.

Is this likely to get merged soon, or is issue 14835 a blocker?

tsbockman · 2015-10-26T22:54:03Z

I added a couple of "compilable" tests.

The test for the dchar fix requires the new VRP-based comparison folding in order to work, demonstrating why the two belong in the same PR.

Although it is not my motivation, I noticed that this PR is a partial fix for issue 13010.

tsbockman · 2015-10-28T18:13:31Z

I made a forum thread to discuss fixing issue 14385.

There is no consensus that it is even a bug; several people argued that the current behaviour is correct. In any case, it is not obvious that there is a good way to fix it short of just removing the warning.

I do not think 14385 should block this PR. Are there other objections?

tsbockman · 2016-01-12T08:46:40Z

Updated whitespace.

ibuclaw · 2016-01-17T11:03:32Z

src/constfold.d

-        assert(0);
-    }
-    else
+    else if (e1.isConst() == 1 && e2.isConst() == 1)


Doesn't isConst return a bool? Seems strange to explicitly compare it's return value.

It does not. It returns 2 for some stuff that isn't const but can be accessed in ctfe.

tsbockman · 2016-01-17T15:02:46Z

Note that the auto-tester failure is due to DMD 14835 again.

tsbockman · 2016-01-25T01:05:04Z

The test for the dchar fix requires the new VRP-based comparison folding in order to work, demonstrating why the two belong in the same PR.

@quickfur I'll split the dchar fix into a separate PR, if you can provide me with a simple, clear test case that fails on the current compiler, without the rest of this PR.

mathias-lang-sociomantic · 2016-03-16T22:29:25Z

@tsbockman @lionello : Thank you so much for working on this. It is IMO a change of great importance for the end user.

I agree that https://issues.dlang.org/show_bug.cgi?id=14835 is a big annoyance, and can only get worse in the future. However, in this case, it is actually helpful. (EDIT: Actually, after testing, I couldn't find any case where it was).
The chance we have is, it triggers a warning, not an error. Someone updating has the possibility to temporarily switch from warning as error to informational ones, until their dependencies are fixed, so that shouldn't be considered a blocker.

However, changing dchar.max should probably be rolled out before, and if possible in a different release. Did you raise a P.R. for this ?

tsbockman · 2016-03-16T22:36:44Z

However, in this case, it is actually helpful.

How so?

In general, there is absolutely nothing wrong with the code that triggers that warning with my VRP improvements. The warning is essentially spurious. There is nothing to "fix" - it's just noise that must be ignored or worked around, in a very tedious fashion.

However, changing dchar.max should probably be rolled out before, and if possible in a different release. Did you raise a P.R. for this ?

I'd like to, but I haven't found any reliable way of actually demonstrating the problem without the rest of this patch. (That's why it hasn't been a big problem so far: the compiler is not yet "smart enough" for the faulty assumption to cause real problems.)

I'm not really comfortable submitting a "fix" without a test case to go with it.

mathias-lang-sociomantic · 2016-03-16T23:21:26Z

I'd like to, but I haven't found any reliable way of actually demonstrating the problem without the rest of this patch. (That's why it hasn't been a big problem so far: the compiler is not yet "smart enough" for the faulty assumption to cause real problems.)

I'm not really comfortable submitting a "fix" without a test case to go with it.

It has never been a problem to submit separate P.R. like this.
I agree a test case would be better, but this change has already been approved and is IMO more a fix to the specs than a fix for VRP. After all, who would ever expect the following to ever fail:

void check (T) (T a) { assert(a <= T.max); }

tsbockman · 2016-03-20T07:35:01Z

@CyberShadow

how about enabling the warning only for non-templated functions that don't have any conditional compilation statements in them? Most non-library D code is not templated anyway.

This possibility is definitely worth looking into. The most important part would be to suppress the warning in template functions, and functions that are nested inside of templates (even if the function itself is not templated).

I'm not sure if scanning the body for conditional compilation statements is necessary or not; spurious warnings can occur outside of templates - even in code with no static if or version blocks - but probably wouldn't very often in practice, even with really good constant folding and VRP.

Also I wonder if the warning could be moved into DScanner.

I'd be very surprised if DScanner can do this better than the compiler, but would be happy to be proven wrong. At least if the warning were coming from DScanner, people wouldn't feel obligated to "fix" the code when it's not actually broken.

EDIT: Looking back, the "goto fail" case is simple enough that a linter like DScanner should be able to detect it without too much trouble - especially since the bad code formatting, alone, is sufficient cause to issue a warning there.

I don't understand this example. Essentially that block of code is turned off (same as version(none)), why would it generate a warning?

It doesn't, and shouldn't, generate a warning. But, some others I've discussed it with expect it to anyway - maybe because of confusion about the precise difference between run-time if, and static if?

tsbockman · 2016-06-16T15:43:48Z

This possibility is definitely worth looking into. The most important part would be to suppress the warning in template functions, and functions that are nested inside of templates (even if the function itself is not templated).

I still believe this is doable, but I don't understand the compiler internals well enough to easily do it myself. Since I am ending my participation in the dlang project, I'm going to close this now.

It would probably just be rejected anyway, since WalterBright doesn't seem the slightest bit interested in the long-term goal of fixing issue 259 (despite having pre-approved the fix).

WalterBright · 2016-06-16T20:53:39Z

This is valuable work. I haven't been able to get to it (I'm currently working on all the complaints about @safe problems).

tsbockman · 2016-06-16T22:12:10Z

@WalterBright

This is valuable work. I haven't been able to get to it

Well then, the aspect of this that really needs your input is how to mitigate issue 14385, which I consider to be a blocker for this.

If that is dealt with, the rest of this should be fairly straightforward, as most of the work was already done by @lionello ages ago.

WalterBright · 2016-06-16T22:40:11Z

I'll look into it.

mathias-lang-sociomantic · 2016-10-12T17:15:46Z

src/intrange.d

-        else if (!isUnsigned)
+        /* Although dchar.max is officially 0x10FFFF, do *NOT* initialize upper to this value.
+           Doing so will cause attempts to guard against invalid out-of-range dchar values to
+           be considered unreachable. */


So with this change, dchar.max will still be 0x10FFFF ?

Yes. Raising dchar.max would be a massive breaking change, and is not needed for my purposes.

All I did is remove the special casing for dchar in the VRP code, so this fix will remain correct even if someone else gets permission to actually set dchar.max == unit.max later.

dlang-bot · 2017-04-21T15:10:31Z

Fix	Bugzilla	Description
✗	14835	Statement is not reachable doesn't play along generic code

tsbockman · 2017-04-21T18:06:22Z

Ping @WalterBright . Any update on issue 14385? Is this PR going anywhere, or not?

tsbockman · 2017-05-23T02:52:07Z

This still isn't going anywhere, so I'm moving on.

WalterBright · 2021-03-28T07:01:51Z

The submitter moving on is not a sufficient reason to close good work.

dlang-bot · 2021-03-28T07:01:54Z

Thanks for your pull request and interest in making D better, @tsbockman! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	14835	major	Constant folding should not affect front end flow analysis

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#5229"

ibuclaw · 2021-03-28T07:22:34Z

The submitter moving on is not a sufficient reason to close good work.

Should have added the phantom zone label.

WalterBright · 2021-03-28T07:46:50Z

Nah, I think this is of more immediate value.

tsbockman mentioned this pull request Oct 24, 2015

Trivial fixes to enable Phobos to compile properly with DMD PR #5229. dlang/phobos#3767

Merged

tsbockman force-pushed the constfold_cmp branch from 27a8679 to e00c100 Compare October 24, 2015 04:01

tsbockman force-pushed the constfold_cmp branch from e00c100 to 0446c50 Compare October 24, 2015 16:32

WalterBright reviewed Oct 25, 2015
View reviewed changes

tsbockman force-pushed the constfold_cmp branch from 0446c50 to 70119d8 Compare October 26, 2015 16:27

This was referenced Nov 16, 2015

Bug 259 Comparing signed to unsigned does not generate an error #1913

Closed

Issue 13010: use VRP for static asserts #3706

Closed

tsbockman force-pushed the constfold_cmp branch from a32474a to 80b1231 Compare December 6, 2015 00:02

tsbockman force-pushed the constfold_cmp branch from 80b1231 to e84b1a1 Compare January 12, 2016 08:45

ibuclaw reviewed Jan 17, 2016
View reviewed changes

tsbockman mentioned this pull request Jan 20, 2016

Fix Phobos Issue 15586 dlang/phobos#3943

Merged

tsbockman force-pushed the constfold_cmp branch 3 times, most recently from be32621 to d3e7577 Compare March 16, 2016 23:10

tsbockman force-pushed the constfold_cmp branch from d3e7577 to 7ded4e4 Compare March 16, 2016 23:24

CyberShadow mentioned this pull request Mar 29, 2016

Add warning categories #5592

Closed

tsbockman closed this Jun 16, 2016

WalterBright reopened this Jun 16, 2016

tsbockman force-pushed the constfold_cmp branch 2 times, most recently from b5da01b to 54c4c82 Compare October 10, 2016 18:12

mathias-lang-sociomantic reviewed Oct 12, 2016

View reviewed changes

tsbockman added 3 commits April 21, 2017 06:59

Fix dchar VRP max so RT checks for bad values don't "optimize" away.

3408d5b

Add VRP-based constant folding of integer comparisons.

4670a7a

Disable "statement is not reachable" warning due to DMD bug dlang#14835.

2614955

tsbockman force-pushed the constfold_cmp branch from 54c4c82 to 2614955 Compare April 21, 2017 15:10

tsbockman closed this May 23, 2017

WalterBright reopened this Mar 28, 2021

tsbockman mentioned this pull request Mar 29, 2021

fix Issue 14835 - Constant folding should not affect front end flow a… #12311

Open

dlang-bot added Review:Needs Rebase Review:Needs Work Review:stalled labels May 20, 2021

dlang-bot added needs rebase stalled labels Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VRP-based constant folding for integer comparisons. #5229

Add VRP-based constant folding for integer comparisons. #5229

tsbockman commented Oct 24, 2015

JackStouffer commented Oct 24, 2015

tsbockman commented Oct 24, 2015

tsbockman commented Oct 24, 2015

lionello commented Oct 24, 2015

tsbockman commented Oct 24, 2015

tsbockman commented Oct 24, 2015

WalterBright Oct 25, 2015

tsbockman commented Oct 26, 2015

tsbockman commented Oct 26, 2015

tsbockman commented Oct 28, 2015

tsbockman commented Jan 12, 2016

ibuclaw Jan 17, 2016

yebblies Jan 17, 2016

tsbockman commented Jan 17, 2016

tsbockman commented Jan 25, 2016

mathias-lang-sociomantic commented Mar 16, 2016

tsbockman commented Mar 16, 2016

mathias-lang-sociomantic commented Mar 16, 2016

tsbockman commented Mar 20, 2016

tsbockman commented Jun 16, 2016

WalterBright commented Jun 16, 2016

tsbockman commented Jun 16, 2016 •

edited

Loading

WalterBright commented Jun 16, 2016

mathias-lang-sociomantic Oct 12, 2016

tsbockman Oct 12, 2016 •

edited

Loading

dlang-bot commented Apr 21, 2017

tsbockman commented Apr 21, 2017

tsbockman commented May 23, 2017

WalterBright commented Mar 28, 2021

dlang-bot commented Mar 28, 2021

ibuclaw commented Mar 28, 2021

WalterBright commented Mar 28, 2021

Add VRP-based constant folding for integer comparisons. #5229

Are you sure you want to change the base?

Add VRP-based constant folding for integer comparisons. #5229

Conversation

tsbockman commented Oct 24, 2015

JackStouffer commented Oct 24, 2015

tsbockman commented Oct 24, 2015

tsbockman commented Oct 24, 2015

lionello commented Oct 24, 2015

tsbockman commented Oct 24, 2015

tsbockman commented Oct 24, 2015

WalterBright Oct 25, 2015

Choose a reason for hiding this comment

tsbockman commented Oct 26, 2015

tsbockman commented Oct 26, 2015

tsbockman commented Oct 28, 2015

tsbockman commented Jan 12, 2016

ibuclaw Jan 17, 2016

Choose a reason for hiding this comment

yebblies Jan 17, 2016

Choose a reason for hiding this comment

tsbockman commented Jan 17, 2016

tsbockman commented Jan 25, 2016

mathias-lang-sociomantic commented Mar 16, 2016

tsbockman commented Mar 16, 2016

mathias-lang-sociomantic commented Mar 16, 2016

tsbockman commented Mar 20, 2016

tsbockman commented Jun 16, 2016

WalterBright commented Jun 16, 2016

tsbockman commented Jun 16, 2016 • edited Loading

WalterBright commented Jun 16, 2016

mathias-lang-sociomantic Oct 12, 2016

Choose a reason for hiding this comment

tsbockman Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

dlang-bot commented Apr 21, 2017

tsbockman commented Apr 21, 2017

tsbockman commented May 23, 2017

WalterBright commented Mar 28, 2021

dlang-bot commented Mar 28, 2021

Bugzilla references

Testing this PR locally

ibuclaw commented Mar 28, 2021

WalterBright commented Mar 28, 2021

tsbockman commented Jun 16, 2016 •

edited

Loading

tsbockman Oct 12, 2016 •

edited

Loading