[COMMITTED,3/3] Update bitwise_or op_range.
Checks
Commit Message
When experimenting with more complex flow for the assume pass, I found a
case where we were not as precise as we should be, but it was because
range-ops hadn't been taught that if the LHS of a bitwise OR was
positive, that the 2 RHS operands must also be positive.
The testcase is simple a tweaked version of an existing testcase which
turned
if (a_1 < 0 && b_2 < 0) goto bb3; else goto bb4;
into
_4 = a_1 | b_2
if (_4 < 0) goto bb3; else goto bb4;
on the branch to bb4, the condition is _4 >= 0 and we were not getting
positive values for a_1 and b_2 on that edge resulting in ranges that
included [-INF, -1] that shouldn't have been there.
This patch fixes that by implementing this functionality in op1_range in
operator_bitwise_or, and adds the testcase.
Bootstrapped on x86_64-pc-linux-gnu with no regressions. Pushed.
Andrew
Comments
On Fri, Nov 01, 2024 at 03:25:37PM -0400, Andrew MacLeod wrote:
> When experimenting with more complex flow for the assume pass, I found a
> case where we were not as precise as we should be, but it was because
> range-ops hadn't been taught that if the LHS of a bitwise OR was positive,
> that the 2 RHS operands must also be positive.
>
> The testcase is simple a tweaked version of an existing testcase which
> turned
>
> if (a_1 < 0 && b_2 < 0) goto bb3; else goto bb4;
>
> into
> _4 = a_1 | b_2
> if (_4 < 0) goto bb3; else goto bb4;
>
> on the branch to bb4, the condition is _4 >= 0 and we were not getting
> positive values for a_1 and b_2 on that edge resulting in ranges that
> included [-INF, -1] that shouldn't have been there.
>
> This patch fixes that by implementing this functionality in op1_range in
> operator_bitwise_or, and adds the testcase.
Does the code handle properly also the case of if lhs of signed bitwise and
is negative, then both rhs1 and rhs2 are negative?
Jakub
Maybe. The bitwise_and op1_range is very complicated.. I think its
heavily used, so I would be surprised if it didn't. bitwise or was
doing nothing at all and showed up immediately when I tried a few
things. I think i just happned to hit it "lucky". It didnt fail any
existing cases, just the ones I was trying to make more complicated.
Do we want to port this to GCC14? Its pretty straightforward I think..
mostly cosmetic changes due to Value_Range and difference in the
range-query access to gori. Ive got it converted already, but noticed
there are some failures on the gcc14 branch? ie, seems to be:
FAIL: g++.dg/cpp23/attr-assume1.C -std=c++14 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume1.C -std=c++17 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume1.C -std=c++20 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume3.C -std=c++14 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume3.C -std=c++17 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume3.C -std=c++20 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++14 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++17 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++20 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++14 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++17 (test for excess errors)
FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++20 (test for excess errors
Andrew
On 11/1/24 15:45, Jakub Jelinek wrote:
> On Fri, Nov 01, 2024 at 03:25:37PM -0400, Andrew MacLeod wrote:
>> When experimenting with more complex flow for the assume pass, I found a
>> case where we were not as precise as we should be, but it was because
>> range-ops hadn't been taught that if the LHS of a bitwise OR was positive,
>> that the 2 RHS operands must also be positive.
>>
>> The testcase is simple a tweaked version of an existing testcase which
>> turned
>>
>> if (a_1 < 0 && b_2 < 0) goto bb3; else goto bb4;
>>
>> into
>> _4 = a_1 | b_2
>> if (_4 < 0) goto bb3; else goto bb4;
>>
>> on the branch to bb4, the condition is _4 >= 0 and we were not getting
>> positive values for a_1 and b_2 on that edge resulting in ranges that
>> included [-INF, -1] that shouldn't have been there.
>>
>> This patch fixes that by implementing this functionality in op1_range in
>> operator_bitwise_or, and adds the testcase.
> Does the code handle properly also the case of if lhs of signed bitwise and
> is negative, then both rhs1 and rhs2 are negative?
>
> Jakub
>
On Fri, Nov 01, 2024 at 04:00:17PM -0400, Andrew MacLeod wrote:
> Maybe. The bitwise_and op1_range is very complicated.. I think its heavily
> used, so I would be surprised if it didn't. bitwise or was doing nothing
> at all and showed up immediately when I tried a few things. I think i just
> happned to hit it "lucky". It didnt fail any existing cases, just the ones
> I was trying to make more complicated.
>
> Do we want to port this to GCC14? Its pretty straightforward I think..
I think we want it on the trunk for a week or two first.
The reason for backporting is that it is a serious wrong-code and I think we
don't want to disable assume support altogether instead.
> mostly cosmetic changes due to Value_Range and difference in the range-query
> access to gori. Ive got it converted already, but noticed there are some
> failures on the gcc14 branch? ie, seems to be:
>
> FAIL: g++.dg/cpp23/attr-assume1.C -std=c++14 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume1.C -std=c++17 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume1.C -std=c++20 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume3.C -std=c++14 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume3.C -std=c++17 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume3.C -std=c++20 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++17 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++20 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++17 (test for excess errors)
> FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++20 (test for excess errors
That needs to be debugged and fixed before commiting, can help with that
next week if it is in some code I'm familiar with.
And important is what the excess errors are.
Jakub
no, I mean those are on the branch as is... at least when I extracted
and built stage 1 release gcc14 branch. UNless there was something wrong
with my build which I havent verified.. it was just a first cut.
The converted patch does not contribute any extra failures.
Andrew
On 11/1/24 16:06, Jakub Jelinek wrote:
> On Fri, Nov 01, 2024 at 04:00:17PM -0400, Andrew MacLeod wrote:
>> Maybe. The bitwise_and op1_range is very complicated.. I think its heavily
>> used, so I would be surprised if it didn't. bitwise or was doing nothing
>> at all and showed up immediately when I tried a few things. I think i just
>> happned to hit it "lucky". It didnt fail any existing cases, just the ones
>> I was trying to make more complicated.
>>
>> Do we want to port this to GCC14? Its pretty straightforward I think..
> I think we want it on the trunk for a week or two first.
> The reason for backporting is that it is a serious wrong-code and I think we
> don't want to disable assume support altogether instead.
>
>> mostly cosmetic changes due to Value_Range and difference in the range-query
>> access to gori. Ive got it converted already, but noticed there are some
>> failures on the gcc14 branch? ie, seems to be:
>>
>> FAIL: g++.dg/cpp23/attr-assume1.C -std=c++14 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume1.C -std=c++17 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume1.C -std=c++20 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume3.C -std=c++14 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume3.C -std=c++17 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume3.C -std=c++20 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++14 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++17 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume5.C -std=gnu++20 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++14 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++17 (test for excess errors)
>> FAIL: g++.dg/cpp23/attr-assume6.C -std=gnu++20 (test for excess errors
> That needs to be debugged and fixed before commiting, can help with that
> next week if it is in some code I'm familiar with.
> And important is what the excess errors are.
>
> Jakub
>
On Fri, Nov 01, 2024 at 05:00:05PM -0400, Andrew MacLeod wrote:
> no, I mean those are on the branch as is... at least when I extracted and
> built stage 1 release gcc14 branch. UNless there was something wrong with my
> build which I havent verified.. it was just a first cut.
Strange. I certainly don't see those.
https://gcc.gnu.org/pipermail/gcc-testresults/2024-November/829576.html
also doesn't.
All I see as FAILs for g++ tests are:
FAIL: g++.dg/cpp2a/concepts-pr67774.C -std=c++23 (test for excess errors)
FAIL: g++.dg/cpp2a/concepts-pr67774.C -std=c++26 (test for excess errors)
FAIL: g++.dg/guality/pr55665.C -O2 -flto -fno-use-linker-plugin -flto-partition=none line 23 p == 40
(when testing with my usual GXX_TESTSUITE_STDS=98,11,14,17,20,23,26)
Neither Fedora 14.2.1 build a week ago:
https://kojipkgs.fedoraproject.org/packages/gcc/14.2.1/5.fc42/data/logs/x86_64/build.log
Jakub
It eas an existing branch. I will get a scrtach branch and do
everything from scratch the beginning of the week.
IM always suspicious there is something laying around when its been
there a while :-) Who knows what I was doing with it 8 months ago :-)
Let me know if these feels more stable.. it should.
Andrew
On 11/1/24 17:05, Jakub Jelinek wrote:
> On Fri, Nov 01, 2024 at 05:00:05PM -0400, Andrew MacLeod wrote:
>> no, I mean those are on the branch as is... at least when I extracted and
>> built stage 1 release gcc14 branch. UNless there was something wrong with my
>> build which I havent verified.. it was just a first cut.
> Strange. I certainly don't see those.
> https://gcc.gnu.org/pipermail/gcc-testresults/2024-November/829576.html
> also doesn't.
> All I see as FAILs for g++ tests are:
> FAIL: g++.dg/cpp2a/concepts-pr67774.C -std=c++23 (test for excess errors)
> FAIL: g++.dg/cpp2a/concepts-pr67774.C -std=c++26 (test for excess errors)
> FAIL: g++.dg/guality/pr55665.C -O2 -flto -fno-use-linker-plugin -flto-partition=none line 23 p == 40
> (when testing with my usual GXX_TESTSUITE_STDS=98,11,14,17,20,23,26)
> Neither Fedora 14.2.1 build a week ago:
> https://kojipkgs.fedoraproject.org/packages/gcc/14.2.1/5.fc42/data/logs/x86_64/build.log
>
> Jakub
>
Fresh extract and bootstrap, and I dont see those spurious failures I
mnetioned. And everything is good with the port of these patches too on
GCC14. so whenever seems reasonable.
Andrew
On 11/1/24 17:10, Andrew MacLeod wrote:
> It eas an existing branch. I will get a scrtach branch and do
> everything from scratch the beginning of the week.
>
> IM always suspicious there is something laying around when its been
> there a while :-) Who knows what I was doing with it 8 months ago :-)
>
> Let me know if these feels more stable.. it should.
>
> Andrew
>
>
>
> On 11/1/24 17:05, Jakub Jelinek wrote:
>> On Fri, Nov 01, 2024 at 05:00:05PM -0400, Andrew MacLeod wrote:
>>> no, I mean those are on the branch as is... at least when I
>>> extracted and
>>> built stage 1 release gcc14 branch. UNless there was something wrong
>>> with my
>>> build which I havent verified.. it was just a first cut.
>> Strange. I certainly don't see those.
>> https://gcc.gnu.org/pipermail/gcc-testresults/2024-November/829576.html
>> also doesn't.
>> All I see as FAILs for g++ tests are:
>> FAIL: g++.dg/cpp2a/concepts-pr67774.C -std=c++23 (test for excess
>> errors)
>> FAIL: g++.dg/cpp2a/concepts-pr67774.C -std=c++26 (test for excess
>> errors)
>> FAIL: g++.dg/guality/pr55665.C -O2 -flto -fno-use-linker-plugin
>> -flto-partition=none line 23 p == 40
>> (when testing with my usual GXX_TESTSUITE_STDS=98,11,14,17,20,23,26)
>> Neither Fedora 14.2.1 build a week ago:
>> https://kojipkgs.fedoraproject.org/packages/gcc/14.2.1/5.fc42/data/logs/x86_64/build.log
>>
>>
>> Jakub
>>
On Sat, Nov 02, 2024 at 02:47:55PM -0400, Andrew MacLeod wrote:
> Fresh extract and bootstrap, and I dont see those spurious failures I
> mnetioned. And everything is good with the port of these patches too on
> GCC14. so whenever seems reasonable.
Let's commit the backport now then.
Jakub
From 9283b1b3593f708a4fc11fb6dfcf208ab218a5d9 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Thu, 31 Oct 2024 14:07:00 -0400
Subject: [PATCH 3/3] Update bitwise_or op_range.
If the LHS of a bitwise OR is positive, then so are both operands when
using op1_range or op2_range.
gcc/
* range-op.cc (operator_bitwise_or::op1_range): If LHS is signed
positive, so are both operands.
gcc/testsuite
* g++.dg/cpp23/attr-assume-opt.C (f2b): Alternate flow test.
---
gcc/range-op.cc | 13 +++++++
gcc/testsuite/g++.dg/cpp23/attr-assume-opt.C | 37 +++++++++++++++++++-
2 files changed, 49 insertions(+), 1 deletion(-)
@@ -3822,6 +3822,19 @@ operator_bitwise_or::op1_range (irange &r, tree type,
r.set_zero (type);
return true;
}
+
+ // if (A < 0 && B < 0)
+ // Sometimes gets translated to
+ // _1 = A | B
+ // if (_1 < 0))
+ // It is useful for ranger to recognize a positive LHS means the RHS
+ // operands are also positive when dealing with the ELSE range..
+ if (TYPE_SIGN (type) == SIGNED && wi::ge_p (lhs.lower_bound (), 0, SIGNED))
+ {
+ unsigned prec = TYPE_PRECISION (type);
+ r.set (type, wi::zero (prec), wi::max_value (prec, SIGNED));
+ return true;
+ }
r.set_varying (type);
return true;
}
@@ -38,5 +38,40 @@ f3 (int x, int y, int z)
return 1;
}
+// This is the same as f2, except there is more complicated flow and
+// required a range-op update to bitwise or.
+
+void barn(int x);
+// assume (x+12 == 14 && y >= 0 && y + 10 < 13 && z + 4 >= 4 && z - 2 < 18)
+// in different order and form with function calls to cause branches.
+bool assume_func (int x, int y, int z)
+{
+ if (z - 2 >= 18)
+ return false;
+ if (x+12 != 14)
+ return false;
+ barn (x);
+ if (y < 0)
+ return false;
+ if (z + 4 < 4)
+ return false;
+ barn (y);
+ if (y + 10 >= 13)
+ return false;
+ barn (z);
+ return true;
+}
+
+int
+f2b (int x, int y, int z)
+{
+ [[assume (assume_func (x, y, z))]];
+ unsigned q = x + y + z;
+ if (q*2 > 46)
+ return 0;
+ return 1;
+}
+
+
/* { dg-final { scan-tree-dump-times "return 0" 0 "vrp2" } } */
-/* { dg-final { scan-tree-dump-times "return 1" 3 "vrp2" } } */
+/* { dg-final { scan-tree-dump-times "return 1" 4 "vrp2" } } */
--
2.45.0