[Chapel Merge] Change `range.size` to return `int`; add `range.si

Notifications Commits
1

Branch: refs/heads/master
Revision: 7ae792c
Author: bradcray
Log Message:

Merge pull request #17628 from bradcray/range-size-as-int

Change range.size to return int; add range.sizeAs()

[reviewed by @ronawho]

This PR changes .size routines on ranges, domains, and arrays to return int by default under the rationale that it's pretty big and convenient as Chapel's default int value (see #17643 for details). It also adds a .sizeAs() routine for more explicitly getting the size as a specific integer type. These changes have some ripple effects, such as using integers to store internal fields relating to an array's sizes (which is good because a multidimensional array with idxType of int(8) could've easily overflowed these values before) and changing shape() queries on domains and arrays to similarly return tuples of int rather than idxType.

The main pain of this PR is that in order to avoid pulling the rug out from users, we want to gently deprecate the old behavior. At the same time, we want to give users the ability to get their code ready for the new world. As a result, a config param has been added which allows one to opt into the new behavior now rather than waiting. When this config is unset, and the user is using a non-int idxType, they will get a warning letting them know about the change. To avoid this warning, they either need to opt into the config or switch to the .sizeAs() routine (or squash all warnings).

The impact of this warning is that all uses of .size on ranges, domains, and arrays within our module code needed to change to sizeAs() routines to ensure that they would not get the warning. And this is a bit nerve-wracking because it's hard to feel confident that we got all such calls since the warnings will only be generated for code paths our tests cover; and because there are other .size queries on tuples, lists, etc. that did not need to change.

One specific case that is not currently handled is that the .shape routine does not have a user-facing .shapeAs() equivalent (primarily because it seems like silly overkill), yet library routines like LinearAlgebra use the .shape() query, so could generate errors if users call the routines on domains/arrays whose idxType is not int. This seems sufficiently unlikely to me that I did not put any effort into it, but will check with @ben-albrecht and others to see whether they agree.

The other slightly challenging decision that had to be made in this transformation was what types module uses of `.sizeAs() should use, where the most obvious choices would be:

  • .sizeAs(intIdxType): matches previous behavior, so is no worse than what we had (though we know there were cases where this was insufficient in the past, such as #17264 and #7530).
  • .sizeAs(int): seems attractive in that it will be the new default once we remove the old behavior; yet is potentially not big enough for all range(uint) values (it's also potentially not big enough for domain(uint) and arrays over such domains, though this seems much less likely since there will typically be arrays of that size, and that's a pretty huge array).
  • .sizeAs(uint): has the benefit of always being big enough, but the downside of using uints which can be annoying in Chapel

In practice, the heuristic I typically took was to try and use .sizeAs(uint) when I thought it was likely/probable that the size would conservatively need max(uint) values; .sizeAs(int) in other cases; and size(idxType) when I felt the old behavior was reasonable or just didn't want to worry about it for now. I strongly suspect that if one computed extensively with non-int ranges, domains, and arrays, weaknesses in these choices could be found, though I'm not sure that things are much worse than they were before (where, already, using uint indices could often lead to problems), and have a slight hope that I've improved some things.

Not directly related to this change, but incorporated as a result of reviewer feedback, I also refactored the .safeCast() routine so that when checks are on, the amount of code inlined is much smaller by refactoring a lot of the bounds checking code into a helper routine. This was motivated in this code by my reliance on a greater number of .safeCast() calls in the DR array implementation.

The changes involved in this are as follows:

Updated ChapelRange:

  • added the sizeReturnsInt config to alternate between the old and new behaviors
  • updated .size to work as described above
  • added range.sizeAs()
  • changed cases of .size to one of size(idxType) or size(uint) based on context
  • fixed a case of bad recursion for the .high routine for single-value idxTypes by reasoning about _low and _high rather than .size

Updated ChapelArray:

  • implemented similar .size/.sizeAs routines for domains and arrays
  • similarly changed calls to .size into .sizeAs()
  • changed .shape routines to have similar "warn" / "permit using rank*int" using the same sizeReturnsInt config
  • added a .chpl_shapeAs() routine for internal uses of shape() that want to avoid the warning but wasn't convinced we'd want to support this long-term, so didn't expose it to the user

Updated storage of DefaultRectangular:

  • changed block multipliers, origin, and factoredOffs from intIdxType to simply int (large, convenient, supports signed and unsigned)
  • similarly, generally changed math used to compute offsets into an array's buffer from using intIdxType to using int
  • changed uses of .size to .sizeAs()

For array implementation / range utility code:

  • changed multipliers / address math to generally use int types rather than idxType
  • changed fairly general purpose .size calls to .sizeAs() or to avoid warnings
  • changed .size:someIntType patterns into .sizeAs(someIntType)

...in these files:

  • modules/dists/BlockDist.chpl
  • modules/dists/CyclicDist.chpl
  • modules/dists/DSIUtil.chpl
  • modules/dists/DimensionalDist2D.chpl
  • modules/dists/ReplicatedDist.chpl
  • modules/dists/dims/BlockCycDim.chpl
  • modules/dists/dims/BlockDim.chpl
  • modules/dists/dims/ReplicatedDim.chpl
  • modules/internal/ArrayViewReindex.chpl
  • modules/packages/RangeChunk.chpl
  • modules/standard/DynamicIters.chpl

In Random.chpl:

  • changed .size calls into .sizeAs() calls

Change the compiler's lowering of coforall loops to use a helper routine chpl_coforallSize to compute the size of a loop rather than relying on a direct call to .size() to avoid the warning that we generate for .size() calls today:

  • compiler/AST/build.cpp
  • modules/internal/ChapelBase.chpl

Updated test to fix creation of expected result when size == 0 (previously had been passing due to .size wrapping around):

  • test/distributions/vass/densify-1-self.chpl

Added compopts file to switch to new behavior early:

  • test/distributions/robust/arithmetic/trivial/test_dot_numIndices.compopts
  • test/types/range/diten/testRangeLength.compopts
  • test/types/range/elliot/rangeLength/intRangeLength.compopts
  • test/types/range/elliot/rangeLength/rangeLengthOverflow.compopts
  • test/types/range/elliot/rangeLength/uintRangeLength.compopts
  • test/types/range/userAPI/COMPOPTS

Retired future that now works and created a variant that tests generated error messages:

  • test/types/range/elliot/rangeLength/rangeLengthOverflow.bad
  • test/types/range/elliot/rangeLength/rangeLengthOverflow2.*

Updated test to keep it working better than before:

  • test/types/range/elliot/rangeLength/uintRangeLength.*

Added test to lock in that .size generates int:

  • test/types/range/sizeType.chpl

Updated range API test to test .sizeAs() routine:

  • test/types/range/userAPI/*

Updated array API test to lock in new warnings:

  • test/arrays/userAPI/unsignedOps.good

Added a test locking in the resolution of #17264:

  • test/arrays/bugs/sizeExceedsIntIdxType.*

Added a test to remind me to remove the old behavior:

  • test/deprecated/rangeSize.chpl

Updated .size calls for non-int idxType tests to use .sizeAs() instead:

  • test/library/packages/Sort/correctness/shellSortExtremeInds.chpl

Updated .size calls for non-int idxType tests to use .sizeAs() instead, and also addressed a reliance on signed int overflow:

  • test/library/packages/Sort/correctness/sortAllIdxTypes.chpl

Refactor .safeCast() to reduce inlined size when checks are on:

  • modules/standard/Types.chpl
  • test/modules/sungeun/init/printModuleInitOrder*.good
  • test/optimizations/deadCodeElimination/elliot/countDeadModules*.good

Resolves #7530
Resolves #17264

Modified Files:
A test/arrays/bugs/sizeExceedsIntIdxType.chpl

A test/arrays/bugs/sizeExceedsIntIdxType.good
A test/deprecated/rangeSize.chpl
A test/deprecated/rangeSize.good
A test/distributions/robust/arithmetic/trivial/test_dot_numIndices.compopts
A test/types/range/diten/testRangeLength.compopts
A test/types/range/elliot/rangeLength/intRangeLength.compopts
A test/types/range/elliot/rangeLength/rangeLengthOverflow.compopts
A test/types/range/elliot/rangeLength/rangeLengthOverflow2-int16.good
A test/types/range/elliot/rangeLength/rangeLengthOverflow2-int32.good
A test/types/range/elliot/rangeLength/rangeLengthOverflow2-int64.good
A test/types/range/elliot/rangeLength/rangeLengthOverflow2-int8.good
A test/types/range/elliot/rangeLength/rangeLengthOverflow2-uint16.good
A test/types/range/elliot/rangeLength/rangeLengthOverflow2-uint32.good
A test/types/range/elliot/rangeLength/rangeLengthOverflow2-uint64.good
A test/types/range/elliot/rangeLength/rangeLengthOverflow2-uint8.good
A test/types/range/elliot/rangeLength/rangeLengthOverflow2.chpl
A test/types/range/elliot/rangeLength/rangeLengthOverflow2.compopts
A test/types/range/elliot/rangeLength/uintRangeLength.compopts
A test/types/range/sizeType-int.good
A test/types/range/sizeType.chpl
A test/types/range/sizeType.compopts
A test/types/range/sizeType.good
A test/types/range/userAPI/COMPOPTS
R test/types/range/elliot/rangeLength/rangeLengthOverflow.bad
R test/types/range/elliot/rangeLength/rangeLengthOverflow.future
M compiler/AST/build.cpp
M modules/dists/BlockDist.chpl
M modules/dists/CyclicDist.chpl
M modules/dists/DSIUtil.chpl
M modules/dists/DimensionalDist2D.chpl
M modules/dists/ReplicatedDist.chpl
M modules/dists/dims/BlockCycDim.chpl
M modules/dists/dims/BlockDim.chpl
M modules/dists/dims/ReplicatedDim.chpl
M modules/internal/ArrayViewReindex.chpl
M modules/internal/ChapelArray.chpl
M modules/internal/ChapelBase.chpl
M modules/internal/ChapelRange.chpl
M modules/internal/DefaultRectangular.chpl
M modules/packages/RangeChunk.chpl
M modules/standard/DynamicIters.chpl
M modules/standard/Random.chpl
M modules/standard/Types.chpl
M test/arrays/userAPI/unsignedOps.good
M test/distributions/vass/densify-1-self.chpl
M test/library/packages/Sort/correctness/shellSortExtremeInds.chpl
M test/library/packages/Sort/correctness/sortAllIdxTypes.chpl
M test/modules/sungeun/init/printModuleInitOrder.good
M test/modules/sungeun/init/printModuleInitOrder.na-none.good
M test/optimizations/deadCodeElimination/elliot/countDeadModules.comm-ofi.good
M test/optimizations/deadCodeElimination/elliot/countDeadModules.comm-ugni.good
M test/optimizations/deadCodeElimination/elliot/countDeadModules.good
M test/types/range/elliot/rangeLength/rangeLengthOverflow.good
M test/types/range/elliot/rangeLength/uintRangeLength.chpl
M test/types/range/elliot/rangeLength/uintRangeLength.good
M test/types/range/userAPI/byteRangeTest.good
M test/types/range/userAPI/codepointRangeTest.good
M test/types/range/userAPI/rangeAPItest.chpl
M test/types/range/userAPI/simpleRangeTest.good
M test/types/range/userAPI/singleEnumAPITest.good

Compare: https://github.com/chapel-lang/chapel/compare/31d5a48254b9...7ae792c400fc
2

Oh shoot, this was reviewed by @mppf as well, but I was going too fast
while typing up the merge message. Sorry, and thanks, Michael!

-Brad