[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC359012: [APSInt][OpenMP] Fix isNegative, etc. for unsigned 
types (authored by jdenny, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59712?vs=195626=196274#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712

Files:
  test/CodeGen/alloc-size.c
  test/OpenMP/distribute_collapse_messages.cpp
  test/OpenMP/distribute_parallel_for_collapse_messages.cpp
  test/OpenMP/distribute_parallel_for_simd_collapse_messages.cpp
  test/OpenMP/distribute_parallel_for_simd_safelen_messages.cpp
  test/OpenMP/distribute_parallel_for_simd_simdlen_messages.cpp
  test/OpenMP/distribute_simd_collapse_messages.cpp
  test/OpenMP/distribute_simd_safelen_messages.cpp
  test/OpenMP/distribute_simd_simdlen_messages.cpp
  test/OpenMP/for_collapse_messages.cpp
  test/OpenMP/for_ordered_clause.cpp
  test/OpenMP/for_simd_collapse_messages.cpp
  test/OpenMP/for_simd_safelen_messages.cpp
  test/OpenMP/for_simd_simdlen_messages.cpp
  test/OpenMP/parallel_for_collapse_messages.cpp
  test/OpenMP/parallel_for_ordered_messages.cpp
  test/OpenMP/parallel_for_simd_collapse_messages.cpp
  test/OpenMP/parallel_for_simd_safelen_messages.cpp
  test/OpenMP/parallel_for_simd_simdlen_messages.cpp
  test/OpenMP/simd_collapse_messages.cpp
  test/OpenMP/simd_safelen_messages.cpp
  test/OpenMP/simd_simdlen_messages.cpp
  test/OpenMP/target_map_messages.cpp
  test/OpenMP/target_parallel_for_collapse_messages.cpp
  test/OpenMP/target_parallel_for_map_messages.cpp
  test/OpenMP/target_parallel_for_ordered_messages.cpp
  test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
  test/OpenMP/target_parallel_for_simd_map_messages.cpp
  test/OpenMP/target_parallel_for_simd_ordered_messages.cpp
  test/OpenMP/target_parallel_for_simd_safelen_messages.cpp
  test/OpenMP/target_parallel_for_simd_simdlen_messages.cpp
  test/OpenMP/target_parallel_map_messages.cpp
  test/OpenMP/target_simd_collapse_messages.cpp
  test/OpenMP/target_simd_safelen_messages.cpp
  test/OpenMP/target_simd_simdlen_messages.cpp
  test/OpenMP/target_teams_distribute_collapse_messages.cpp
  test/OpenMP/target_teams_distribute_map_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_collapse_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_simd_collapse_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_simd_safelen_messages.cpp
  test/OpenMP/target_teams_distribute_parallel_for_simd_simdlen_messages.cpp
  test/OpenMP/target_teams_distribute_simd_collapse_messages.cpp
  test/OpenMP/target_teams_distribute_simd_map_messages.cpp
  test/OpenMP/target_teams_distribute_simd_safelen_messages.cpp
  test/OpenMP/target_teams_distribute_simd_simdlen_messages.cpp
  test/OpenMP/target_teams_map_messages.cpp
  test/OpenMP/taskloop_collapse_messages.cpp
  test/OpenMP/taskloop_simd_collapse_messages.cpp
  test/OpenMP/taskloop_simd_safelen_messages.cpp
  test/OpenMP/taskloop_simd_simdlen_messages.cpp
  test/OpenMP/teams_distribute_collapse_messages.cpp
  test/OpenMP/teams_distribute_parallel_for_collapse_messages.cpp
  test/OpenMP/teams_distribute_parallel_for_simd_collapse_messages.cpp
  test/OpenMP/teams_distribute_parallel_for_simd_safelen_messages.cpp
  test/OpenMP/teams_distribute_parallel_for_simd_simdlen_messages.cpp
  test/OpenMP/teams_distribute_simd_collapse_messages.cpp
  test/OpenMP/teams_distribute_simd_safelen_messages.cpp
  test/OpenMP/teams_distribute_simd_simdlen_messages.cpp
  test/Sema/shift.c
  test/SemaCXX/constexpr-unsigned-high-bit.cpp

Index: test/OpenMP/distribute_parallel_for_collapse_messages.cpp
===
--- test/OpenMP/distribute_parallel_for_collapse_messages.cpp
+++ test/OpenMP/distribute_parallel_for_collapse_messages.cpp
@@ -53,7 +53,7 @@
 #pragma omp distribute parallel for collapse ((ST > 0) ? 1 + ST : 2) // expected-note 2 {{as specified in 'collapse' clause}}
   for (int i = ST; i < N; i++) argv[0][i] = argv[0][i] - argv[0][i-ST]; // expected-error 2 {{expected 2 for loops after '#pragma omp distribute parallel for', but found only 1}}
   // expected-error@+8 2 {{directive '#pragma omp distribute parallel for' cannot contain more than one 'collapse' clause}}
-  // expected-error@+7 2 {{argument to 'collapse' clause must be a strictly positive integer value}}
+  // expected-error@+7 {{argument to 'collapse' clause must be a strictly positive integer value}}
   // expected-error@+6 2 {{expression is not an integral constant expression}}
 #if __cplusplus >= 201103L
   // expected-note@+4 2 {{non-constexpr function 'foobool' cannot be used in a constant expression}}
@@ -124,7 +124,7 @@
   // expected-note@+6{{non-constexpr function 'foobool' cannot be used in 

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for the reviews!  Will push soon.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

The `APSInt.h` itself looks good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Hal Finkel via Phabricator via cfe-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

In D59712#1473223 , @jdenny wrote:

> In D59712#1469392 , @lebedev.ri 
> wrote:
>
> > Does this pass `check-all`? `check-all` of stage-2? test-suite?
>
>
> For me, all these tests behave with the current patch.  As before, the only 
> subproject I did not build was llgo.


Great. LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D59712#1469392 , @lebedev.ri wrote:

> Does this pass `check-all`? `check-all` of stage-2? test-suite?


For me, all these tests behave with the current patch.  As before, the only 
subproject I did not build was llgo.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D59712#1472544 , @jdenny wrote:

> In D59712#1472358 , @hfinkel wrote:
>
> > > I've never tried running the other tests you mention, for any patch.  I 
> > > thought people normally left those to the bots.  Should this patch be 
> > > handled differently?
> >
> > We have a lot of people actively working off of trunk, and we try very hard 
> > to keep trunk clean. The bots are a second line of defense, not the primary 
> > checkers. In any case, this comes down to professional judgement. It is not 
> > uncommon to ask for a patch author to check self hosting and a test suite 
> > run before committing - specifically, those patches that might affect 
> > correctness, or introduce other subtle problems, and for which running the 
> > compiler over a bunch of C/C++ code might uncover a problem.
>
>
> Thanks for explaining.  It's my first time receiving these particular 
> requests (probably because of what parts of LLVM I normally edit), so I 
> wasn't sure I understood.


No problem.

> For self-hosting, is it best to build again with CMAKE_C_COMPILER and 
> CMAKE_CXX_COMPILE pointing into the previous build, or is there a better 
> approach?

That's what I do.

> 
> 
>> Also, is this review now missing some files? I see here only updates to  
>> APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. 
>> Nothing that would cause changes to the tests, however (maybe I'm just 
>> missing something).
> 
> All looks fine to me.  The APSInt.h changes are the reason for all the test 
> changes.

Indeed. I forgot that you were changing overrides.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D59712#1472358 , @hfinkel wrote:

> > I've never tried running the other tests you mention, for any patch.  I 
> > thought people normally left those to the bots.  Should this patch be 
> > handled differently?
>
> We have a lot of people actively working off of trunk, and we try very hard 
> to keep trunk clean. The bots are a second line of defense, not the primary 
> checkers. In any case, this comes down to professional judgement. It is not 
> uncommon to ask for a patch author to check self hosting and a test suite run 
> before committing - specifically, those patches that might affect 
> correctness, or introduce other subtle problems, and for which running the 
> compiler over a bunch of C/C++ code might uncover a problem.


Thanks for explaining.  It's my first time receiving these particular requests 
(probably because of what parts of LLVM I normally edit), so I wasn't sure I 
understood.

For self-hosting, is it best to build again with CMAKE_C_COMPILER and 
CMAKE_CXX_COMPILE pointing into the previous build, or is there a better 
approach?

> Also, is this review now missing some files? I see here only updates to  
> APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. 
> Nothing that would cause changes to the tests, however (maybe I'm just 
> missing something).

All looks fine to me.  The APSInt.h changes are the reason for all the test 
changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D59712#1472318 , @jdenny wrote:

> In D59712#1469760 , @lebedev.ri 
> wrote:
>
> > In D59712#1469693 , @jdenny wrote:
> >
> > > In D59712#1469392 , @lebedev.ri 
> > > wrote:
> > >
> > > > In D59712#1469358 , @jdenny 
> > > > wrote:
> > > >
> > > > > In D59712#1469301 , 
> > > > > @lebedev.ri wrote:
> > > > >
> > > > > > In D59712#1469295 , 
> > > > > > @craig.topper wrote:
> > > > > >
> > > > > > > Wondering if it would be better to assert for asking for the sign 
> > > > > > > of an unsigned APSInt. I could see a caller just wanting to get 
> > > > > > > the msb for some reason and not knowing that isNegative won’t 
> > > > > > > work.
> > > > > >
> > > > > >
> > > > > > Yes, i, too, would think an assert is much better solution (since i 
> > > > > > literally just tripped over this in this review.)
> > > > >
> > > >
> > > >
> > > > Does this pass `check-all`? `check-all` of stage-2? test-suite?
> > >
> > >
> > > No.  The assert breaks cases in at least ExprConstant.cpp and 
> > > SemaExpr.cpp.
> >
> >
> > Err, i was talking about the current code in the patch :)
>
>
> For me, check-all's success is not affected by the current patch.  I built 
> all subprojects except llgo, which gave me a build problem independently of 
> this patch.
>
> I've never tried running the other tests you mention, for any patch.  I 
> thought people normally left those to the bots.  Should this patch be handled 
> differently?


We have a lot of people actively working off of trunk, and we try very hard to 
keep trunk clean. The bots are a second line of defense, not the primary 
checkers. In any case, this comes down to professional judgement. It is not 
uncommon to ask for a patch author to check self hosting and a test suite run 
before committing - specifically, those patches that might affect correctness, 
or introduce other subtle problems, and for which running the compiler over a 
bunch of C/C++ code might uncover a problem.

Also, is this review now missing some files? I see here only updates to  
APSInt.h (only adding functions), APSIntTest.cpp, and a bunch of tests. Nothing 
that would cause changes to the tests, however (maybe I'm just missing 
something).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D59712#1472342 , @lebedev.ri wrote:

> In D59712#1472318 , @jdenny wrote:
>
> > For me, check-all's success is not affected by the current patch.  I built 
> > all subprojects except llgo, which gave me a build problem independently of 
> > this patch.
>
>
> (Built natively, or with clang with this patch?)


Built natively.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.

In D59712#1472318 , @jdenny wrote:

> In D59712#1469760 , @lebedev.ri 
> wrote:
>
> > In D59712#1469693 , @jdenny wrote:
> >
> > > In D59712#1469392 , @lebedev.ri 
> > > wrote:
> > >
> > > > In D59712#1469358 , @jdenny 
> > > > wrote:
> > > >
> > > > > In D59712#1469301 , 
> > > > > @lebedev.ri wrote:
> > > > >
> > > > > > In D59712#1469295 , 
> > > > > > @craig.topper wrote:
> > > > > >
> > > > > > > Wondering if it would be better to assert for asking for the sign 
> > > > > > > of an unsigned APSInt. I could see a caller just wanting to get 
> > > > > > > the msb for some reason and not knowing that isNegative won’t 
> > > > > > > work.
> > > > > >
> > > > > >
> > > > > > Yes, i, too, would think an assert is much better solution (since i 
> > > > > > literally just tripped over this in this review.)
> > > > >
> > > >
> > > >
> > > > Does this pass `check-all`? `check-all` of stage-2? test-suite?
> > >
> > >
> > > No.  The assert breaks cases in at least ExprConstant.cpp and 
> > > SemaExpr.cpp.
> >
> >
> > Err, i was talking about the current code in the patch :)
>
>
> For me, check-all's success is not affected by the current patch.  I built 
> all subprojects except llgo, which gave me a build problem independently of 
> this patch.


(Built natively, or with clang with this patch?)

> I've never tried running the other tests you mention, for any patch.  I 
> thought people normally left those to the bots.  Should this patch be handled 
> differently?

Sounds good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D59712#1469760 , @lebedev.ri wrote:

> In D59712#1469693 , @jdenny wrote:
>
> > In D59712#1469392 , @lebedev.ri 
> > wrote:
> >
> > > In D59712#1469358 , @jdenny 
> > > wrote:
> > >
> > > > In D59712#1469301 , 
> > > > @lebedev.ri wrote:
> > > >
> > > > > In D59712#1469295 , 
> > > > > @craig.topper wrote:
> > > > >
> > > > > > Wondering if it would be better to assert for asking for the sign 
> > > > > > of an unsigned APSInt. I could see a caller just wanting to get the 
> > > > > > msb for some reason and not knowing that isNegative won’t work.
> > > > >
> > > > >
> > > > > Yes, i, too, would think an assert is much better solution (since i 
> > > > > literally just tripped over this in this review.)
> > > >
> > >
> > >
> > > Does this pass `check-all`? `check-all` of stage-2? test-suite?
> >
> >
> > No.  The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.
>
>
> Err, i was talking about the current code in the patch :)


For me, check-all's success is not affected by the current patch.  I built all 
subprojects except llgo, which gave me a build problem independently of this 
patch.

I've never tried running the other tests you mention, for any patch.  I thought 
people normally left those to the bots.  Should this patch be handled 
differently?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59712#1469693 , @jdenny wrote:

> In D59712#1469392 , @lebedev.ri 
> wrote:
>
> > In D59712#1469358 , @jdenny wrote:
> >
> > > In D59712#1469301 , @lebedev.ri 
> > > wrote:
> > >
> > > > In D59712#1469295 , 
> > > > @craig.topper wrote:
> > > >
> > > > > Wondering if it would be better to assert for asking for the sign of 
> > > > > an unsigned APSInt. I could see a caller just wanting to get the msb 
> > > > > for some reason and not knowing that isNegative won’t work.
> > > >
> > > >
> > > > Yes, i, too, would think an assert is much better solution (since i 
> > > > literally just tripped over this in this review.)
> > >
> >
> >
> > Does this pass `check-all`? `check-all` of stage-2? test-suite?
>
>
> No.  The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.


Err, i was talking about the current code in the patch :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D59712#1469392 , @lebedev.ri wrote:

> In D59712#1469358 , @jdenny wrote:
>
> > In D59712#1469301 , @lebedev.ri 
> > wrote:
> >
> > > In D59712#1469295 , 
> > > @craig.topper wrote:
> > >
> > > > Wondering if it would be better to assert for asking for the sign of an 
> > > > unsigned APSInt. I could see a caller just wanting to get the msb for 
> > > > some reason and not knowing that isNegative won’t work.
> > >
> > >
> > > Yes, i, too, would think an assert is much better solution (since i 
> > > literally just tripped over this in this review.)
> >
>
>
> Does this pass `check-all`? `check-all` of stage-2? test-suite?


No.  The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.

However, the current version of the patch does not break check-all.  I have not 
tried the others.

Moreover, I see now that this patch fixes behavior for the following:

  #include 
  #include 
  constexpr unsigned foo() {
unsigned i = INT_MAX;
++i; // should not warn value is outside range
return i;
  }
  int main() {
std::cout << foo() << '\n';
std::cout << (1 << (unsigned)-1) << '\n'; // should not warn about neg 
shift count
return 0;
  }

I'll integrate these into the tests later.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59712#1469358 , @jdenny wrote:

> In D59712#1469301 , @lebedev.ri 
> wrote:
>
> > In D59712#1469295 , @craig.topper 
> > wrote:
> >
> > > Wondering if it would be better to assert for asking for the sign of an 
> > > unsigned APSInt. I could see a caller just wanting to get the msb for 
> > > some reason and not knowing that isNegative won’t work.
> >
> >
> > Yes, i, too, would think an assert is much better solution (since i 
> > literally just tripped over this in this review.)
>


Does this pass `check-all`? `check-all` of stage-2? test-suite?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D59712#1469301 , @lebedev.ri wrote:

> In D59712#1469295 , @craig.topper 
> wrote:
>
> > Wondering if it would be better to assert for asking for the sign of an 
> > unsigned APSInt. I could see a caller just wanting to get the msb for some 
> > reason and not knowing that isNegative won’t work.
>
>
> Yes, i, too, would think an assert is much better solution (since i literally 
> just tripped over this in this review.)


Imagine a caller wants to check for a negative value for an APSInt from an 
arbitrary expression.  Whether the value is negative is important, but not why. 
 Given that APSInt is documented as knowing its signedness, it seems 
unintuitive that the caller is responsible for first checking for an unsigned 
type to avoid an internal compiler error.

If the fear is that developers are too used to calling isNegative to check the 
high bit, maybe isNegative should always (regardless of signedness) fail an 
assert for APSInt, and we should find different function names that won't cause 
such confusion.

Whatever we do for isNegative, we should probably do something similar for 
isNonNegative as it has the same issues.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59712#1469295 , @craig.topper 
wrote:

> Wondering if it would be better to assert for asking for the sign of an 
> unsigned APSInt. I could see a caller just wanting to get the msb for some 
> reason and not knowing that isNegative won’t work.


Yes, i, too, would think an assert is much better solution (since i literally 
just tripped over this in this review.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 195452.
jdenny added a comment.

Duplicate new APSInt test but for signed type.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712

Files:
  clang/test/OpenMP/distribute_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_simd_simdlen_messages.cpp
  clang/test/OpenMP/for_collapse_messages.cpp
  clang/test/OpenMP/for_ordered_clause.cpp
  clang/test/OpenMP/for_simd_collapse_messages.cpp
  clang/test/OpenMP/for_simd_safelen_messages.cpp
  clang/test/OpenMP/for_simd_simdlen_messages.cpp
  clang/test/OpenMP/parallel_for_collapse_messages.cpp
  clang/test/OpenMP/parallel_for_ordered_messages.cpp
  clang/test/OpenMP/parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/simd_collapse_messages.cpp
  clang/test/OpenMP/simd_safelen_messages.cpp
  clang/test/OpenMP/simd_simdlen_messages.cpp
  clang/test/OpenMP/target_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/target_parallel_for_ordered_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_ordered_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_simd_collapse_messages.cpp
  clang/test/OpenMP/target_simd_safelen_messages.cpp
  clang/test/OpenMP/target_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_collapse_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_safelen_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_simdlen_messages.cpp
  clang/test/OpenMP/taskloop_collapse_messages.cpp
  clang/test/OpenMP/taskloop_simd_collapse_messages.cpp
  clang/test/OpenMP/taskloop_simd_safelen_messages.cpp
  clang/test/OpenMP/taskloop_simd_simdlen_messages.cpp
  clang/test/OpenMP/teams_distribute_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_simdlen_messages.cpp
  llvm/include/llvm/ADT/APSInt.h
  llvm/unittests/ADT/APSIntTest.cpp

Index: llvm/unittests/ADT/APSIntTest.cpp
===
--- llvm/unittests/ADT/APSIntTest.cpp
+++ llvm/unittests/ADT/APSIntTest.cpp
@@ -159,4 +159,90 @@
 
 #endif
 
+TEST(APSIntTest, SignedHighBit) {
+  APSInt False(APInt(1, 0), false);
+  APSInt True(APInt(1, 1), false);
+  APSInt CharMin(APInt(8, 0), false);
+  APSInt CharSmall(APInt(8, 0x13), false);
+  APSInt CharBoundaryUnder(APInt(8, 0x7f), false);
+  APSInt CharBoundaryOver(APInt(8, 0x80), false);
+  APSInt CharLarge(APInt(8, 0xd9), false);
+  APSInt CharMax(APInt(8, 0xff), false);
+
+  EXPECT_FALSE(False.isNegative());
+  EXPECT_TRUE(False.isNonNegative());
+  EXPECT_FALSE(False.isStrictlyPositive());
+
+  EXPECT_TRUE(True.isNegative());
+  EXPECT_FALSE(True.isNonNegative());
+  EXPECT_FALSE(True.isStrictlyPositive());
+
+  EXPECT_FALSE(CharMin.isNegative());
+  EXPECT_TRUE(CharMin.isNonNegative());
+  EXPECT_FALSE(CharMin.isStrictlyPositive());
+
+  EXPECT_FALSE(CharSmall.isNegative());
+  EXPECT_TRUE(CharSmall.isNonNegative());
+  EXPECT_TRUE(CharSmall.isStrictlyPositive());
+
+  EXPECT_FALSE(CharBoundaryUnder.isNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isNonNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isStrictlyPositive());
+
+  EXPECT_TRUE(CharBoundaryOver.isNegative());
+  EXPECT_FALSE(CharBoundaryOver.isNonNegative());
+  EXPECT_FALSE(CharBoundaryOver.isStrictlyPositive());
+
+  EXPECT_TRUE(CharLarge.isNegative());
+  EXPECT_FALSE(CharLarge.isNonNegative());
+  EXPECT_FALSE(CharLarge.isStrictlyPositive());
+
+  EXPECT_TRUE(CharMax.isNegative());
+  

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Wondering if it would be better to assert for asking for the sign of an 
unsigned APSInt. I could see a caller just wanting to get the msb for some 
reason and not knowing that isNegative won’t work.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.



Comment at: llvm/unittests/ADT/APSIntTest.cpp:162
 
+TEST(APSIntTest, UnsignedHighBit) {
+  APSInt False(APInt(1, 0));

Can you please duplicate this whole test but for signed `APSInt`?



Comment at: llvm/unittests/ADT/APSIntTest.cpp:188-194
+  EXPECT_FALSE(CharBoundaryUnder.isNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isNonNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isStrictlyPositive());
+
+  EXPECT_FALSE(CharBoundaryOver.isNegative());
+  EXPECT_TRUE(CharBoundaryOver.isNonNegative());
+  EXPECT_TRUE(CharBoundaryOver.isStrictlyPositive());

jdenny wrote:
> lebedev.ri wrote:
> > I do not understand.
> > `0x7f` is 127, it is obviously a maximal positive 8-bit value.
> > but `0x80` is 128 aka -128, is it not minimal negative 8-bit value?
> > Is the test correct?
> This test checks that unsigned types are never seen as negative, so it's 
> checking values that would be negative if the type were signed.
Ooh, i see. I was looking at the wrong constructor.
```
  explicit APSInt(APInt I, bool isUnsigned = true)
   : APInt(std::move(I)), IsUnsigned(isUnsigned) {}
```
was the one being called.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added inline comments.



Comment at: llvm/unittests/ADT/APSIntTest.cpp:188-194
+  EXPECT_FALSE(CharBoundaryUnder.isNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isNonNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isStrictlyPositive());
+
+  EXPECT_FALSE(CharBoundaryOver.isNegative());
+  EXPECT_TRUE(CharBoundaryOver.isNonNegative());
+  EXPECT_TRUE(CharBoundaryOver.isStrictlyPositive());

lebedev.ri wrote:
> I do not understand.
> `0x7f` is 127, it is obviously a maximal positive 8-bit value.
> but `0x80` is 128 aka -128, is it not minimal negative 8-bit value?
> Is the test correct?
This test checks that unsigned types are never seen as negative, so it's 
checking values that would be negative if the type were signed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.



Comment at: llvm/unittests/ADT/APSIntTest.cpp:188-194
+  EXPECT_FALSE(CharBoundaryUnder.isNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isNonNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isStrictlyPositive());
+
+  EXPECT_FALSE(CharBoundaryOver.isNegative());
+  EXPECT_TRUE(CharBoundaryOver.isNonNegative());
+  EXPECT_TRUE(CharBoundaryOver.isStrictlyPositive());

I do not understand.
`0x7f` is 127, it is obviously a maximal positive 8-bit value.
but `0x80` is 128 aka -128, is it not minimal negative 8-bit value?
Is the test correct?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks.  Then the patch just needs someone to review from the ADT side.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

I'm fine with the changes in the OpenMP tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Ping.  To be clear, my goal here was not to change the OpenMP behavior.  My 
goal was to fix APSInt behavior.  If people feel that the old OpenMP behavior 
is better somehow, we should surely find another way to implement that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 191938.
jdenny added a comment.

Extend llvm/unittests/ADT/APSIntTest.cpp.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712

Files:
  clang/test/OpenMP/distribute_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_simd_simdlen_messages.cpp
  clang/test/OpenMP/for_collapse_messages.cpp
  clang/test/OpenMP/for_ordered_clause.cpp
  clang/test/OpenMP/for_simd_collapse_messages.cpp
  clang/test/OpenMP/for_simd_safelen_messages.cpp
  clang/test/OpenMP/for_simd_simdlen_messages.cpp
  clang/test/OpenMP/parallel_for_collapse_messages.cpp
  clang/test/OpenMP/parallel_for_ordered_messages.cpp
  clang/test/OpenMP/parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/simd_collapse_messages.cpp
  clang/test/OpenMP/simd_safelen_messages.cpp
  clang/test/OpenMP/simd_simdlen_messages.cpp
  clang/test/OpenMP/target_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/target_parallel_for_ordered_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_ordered_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_simd_collapse_messages.cpp
  clang/test/OpenMP/target_simd_safelen_messages.cpp
  clang/test/OpenMP/target_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_collapse_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_safelen_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_simdlen_messages.cpp
  clang/test/OpenMP/taskloop_collapse_messages.cpp
  clang/test/OpenMP/taskloop_simd_collapse_messages.cpp
  clang/test/OpenMP/taskloop_simd_safelen_messages.cpp
  clang/test/OpenMP/taskloop_simd_simdlen_messages.cpp
  clang/test/OpenMP/teams_distribute_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_simdlen_messages.cpp
  llvm/include/llvm/ADT/APSInt.h
  llvm/unittests/ADT/APSIntTest.cpp

Index: llvm/unittests/ADT/APSIntTest.cpp
===
--- llvm/unittests/ADT/APSIntTest.cpp
+++ llvm/unittests/ADT/APSIntTest.cpp
@@ -159,4 +159,47 @@
 
 #endif
 
+TEST(APSIntTest, UnsignedHighBit) {
+  APSInt False(APInt(1, 0));
+  APSInt True(APInt(1, 1));
+  APSInt CharMin(APInt(8, 0));
+  APSInt CharSmall(APInt(8, 0x13));
+  APSInt CharBoundaryUnder(APInt(8, 0x7f));
+  APSInt CharBoundaryOver(APInt(8, 0x80));
+  APSInt CharLarge(APInt(8, 0xd9));
+  APSInt CharMax(APInt(8, 0xff));
+
+  EXPECT_FALSE(False.isNegative());
+  EXPECT_TRUE(False.isNonNegative());
+  EXPECT_FALSE(False.isStrictlyPositive());
+
+  EXPECT_FALSE(True.isNegative());
+  EXPECT_TRUE(True.isNonNegative());
+  EXPECT_TRUE(True.isStrictlyPositive());
+
+  EXPECT_FALSE(CharMin.isNegative());
+  EXPECT_TRUE(CharMin.isNonNegative());
+  EXPECT_FALSE(CharMin.isStrictlyPositive());
+
+  EXPECT_FALSE(CharSmall.isNegative());
+  EXPECT_TRUE(CharSmall.isNonNegative());
+  EXPECT_TRUE(CharSmall.isStrictlyPositive());
+
+  EXPECT_FALSE(CharBoundaryUnder.isNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isNonNegative());
+  EXPECT_TRUE(CharBoundaryUnder.isStrictlyPositive());
+
+  EXPECT_FALSE(CharBoundaryOver.isNegative());
+  EXPECT_TRUE(CharBoundaryOver.isNonNegative());
+  EXPECT_TRUE(CharBoundaryOver.isStrictlyPositive());
+
+  EXPECT_FALSE(CharLarge.isNegative());
+  EXPECT_TRUE(CharLarge.isNonNegative());
+  EXPECT_TRUE(CharLarge.isStrictlyPositive());
+
+  EXPECT_FALSE(CharMax.isNegative());
+  EXPECT_TRUE(CharMax.isNonNegative());

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D59712#1439966 , @lebedev.ri wrote:

> Might warrant test coverage in `llvm/unittest/ADT/APSIntTest.cpp`.


Thanks.  I'll work on that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Might warrant test coverage in `llvm/unittest/ADT/APSIntTest.cpp`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59712/new/

https://reviews.llvm.org/D59712



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: ABataev, chandlerc, craig.topper.
Herald added subscribers: jdoerfert, dexonsmith, guansong.
Herald added a project: LLVM.

Without this patch, APSInt inherits APInt::isNegative, which merely
checks the sign bit without regard to whether the type is actually
signed.  isNonNegative and isStrictlyPositive call isNegative and so
are also affected.

This patch adjusts APSInt to override isNegative, isNonNegative, and 
isStrictlyPositive with implementations that consider whether the type
is signed.

A large set of Clang OpenMP tests are affected.  Without this patch,
these tests assume that `true` is not a valid argument for clauses
like `collapse`.  Indeed, `true` fails APInt::isStrictlyPositive but 
not APSInt::isStrictlyPositive.  This patch adjusts those tests to
assume `true` should be accepted.


https://reviews.llvm.org/D59712

Files:
  clang/test/OpenMP/distribute_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/distribute_simd_simdlen_messages.cpp
  clang/test/OpenMP/for_collapse_messages.cpp
  clang/test/OpenMP/for_ordered_clause.cpp
  clang/test/OpenMP/for_simd_collapse_messages.cpp
  clang/test/OpenMP/for_simd_safelen_messages.cpp
  clang/test/OpenMP/for_simd_simdlen_messages.cpp
  clang/test/OpenMP/parallel_for_collapse_messages.cpp
  clang/test/OpenMP/parallel_for_ordered_messages.cpp
  clang/test/OpenMP/parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/simd_collapse_messages.cpp
  clang/test/OpenMP/simd_safelen_messages.cpp
  clang/test/OpenMP/simd_simdlen_messages.cpp
  clang/test/OpenMP/target_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/target_parallel_for_ordered_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_ordered_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_simd_collapse_messages.cpp
  clang/test/OpenMP/target_simd_safelen_messages.cpp
  clang/test/OpenMP/target_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_collapse_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_safelen_messages.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_simdlen_messages.cpp
  clang/test/OpenMP/taskloop_collapse_messages.cpp
  clang/test/OpenMP/taskloop_simd_collapse_messages.cpp
  clang/test/OpenMP/taskloop_simd_safelen_messages.cpp
  clang/test/OpenMP/taskloop_simd_simdlen_messages.cpp
  clang/test/OpenMP/teams_distribute_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_safelen_messages.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_simdlen_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_collapse_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_safelen_messages.cpp
  clang/test/OpenMP/teams_distribute_simd_simdlen_messages.cpp
  llvm/include/llvm/ADT/APSInt.h

Index: llvm/include/llvm/ADT/APSInt.h
===
--- llvm/include/llvm/ADT/APSInt.h
+++ llvm/include/llvm/ADT/APSInt.h
@@ -42,6 +42,24 @@
   /// \param Str the string to be interpreted.
   explicit APSInt(StringRef Str);
 
+  /// Determine sign of this APSInt.
+  ///
+  /// \returns true if this APSInt is negative, false otherwise
+  bool isNegative() const { return isSigned() && APInt::isNegative(); }
+
+  /// Determine if this APSInt Value is non-negative (>= 0)
+  ///
+  /// \returns true if this APSInt is non-negative, false otherwise
+  bool isNonNegative() const { return !isNegative(); }
+
+  /// Determine if this APSInt Value is positive.
+  ///
+  /// This tests if the value of this APSInt is positive (> 0). Note
+  /// that 0 is not a positive value.
+  ///
+  /// \returns true if this APSInt is positive.
+  bool isStrictlyPositive() const { return isNonNegative() && !isNullValue(); }
+