Rename range.stridable to range.strides : strideKind
#22441
Conversation
compiler/AST/AggregateType.cpp
Outdated
| if ((!strcmp(ne->name, "boundedType") || !strcmp(ne->name, "stridable")) | ||
| && at->symbol->hasFlag(FLAG_RANGE)) | ||
| { | ||
| if (!strcmp(ne->name, "boundedType")) { |
There was a problem hiding this comment.
Could he nice to factor the repeated strcmp occurrences into boolean variables.
compiler/AST/AggregateType.cpp
Outdated
| while (AggregateType* parentAG = ag->dispatchParents.n == 0 ? | ||
| nullptr : ag->dispatchParents.v[0]) | ||
| { |
There was a problem hiding this comment.
Can ag->dispatchParents.v[0] ever be null if n != 0? If not, perhaps a nicer way to write this is:
| while (AggregateType* parentAG = ag->dispatchParents.n == 0 ? | |
| nullptr : ag->dispatchParents.v[0]) | |
| { | |
| while (ag->dispatchParents.n != 0) | |
| { | |
| AggregateType* parentAG = ag->dispatchParents.v[0] |
compiler/AST/AggregateType.cpp
Outdated
| if ((!strcmp(ne->name, "boundedType") || !strcmp(ne->name, "stridable")) | ||
| && at->symbol->hasFlag(FLAG_RANGE)) | ||
| { |
There was a problem hiding this comment.
Did you mean to use two-space indents here? I just noticed it's only one space...
| static void addRangeDeprecationClone(AggregateType* base, AggregateType* cur, | ||
| FnSymbol* fn1) { | ||
| // the position of the 'stride' field | ||
| int stridesPos = strcmp(base->symbol->name, "BaseRectangularDom") ? 10 : 5; |
There was a problem hiding this comment.
Could these magic numbers 10 and 5 be computed somewhere, just for resilence's sake? Or is the deprecation warning short-term enough that this doesn't matter?
There was a problem hiding this comment.
Computing them is probably more hassle than it is worth. I hope we can remove support for range.stridable soon enough that these numbers will remain correct. If these numbers do change, deprecation tests should catch that.
| else if (pNamed != nullptr && !strcmp(pNamed->name, "strides")) | ||
| pNamed->replace(new NamedExpr("stridable", replSE)); | ||
| else | ||
| se->replace(replSE); |
There was a problem hiding this comment.
This isn't a type-safe replacement, is it? If I have a init proc that feeds strides to something that expects a strideKind, and then it gets replaced with a bool, it won't work? Or is there an implicit conversion to bool somehow (seems unlikely to me).
There was a problem hiding this comment.
This code is invoked only on a clone of the function built by AggregateType::buildDefaultInitializer(). Therefore strides is used only when passing to the parent initializer. In this context, the new stridable formal is safe to use because the parent initializers will most likely also have overloads that accept stridable. Otherwise there will be a compilation failure and the user-defined domain map will need to be ported over. Which is an acceptable outcome.
compiler/passes/normalize.cpp
Outdated
| if (replacedStridable(call, name, use)) | ||
| ; // handled | ||
| else | ||
|
|
There was a problem hiding this comment.
| if (replacedStridable(call, name, use)) | |
| ; // handled | |
| else | |
| if (replacedStridable(call, name, use)) | |
| continue; // handled | |
compiler/passes/normalize.cpp
Outdated
| // Supports deprecation by Vass in 1.31 to implement #17131. | ||
| // chpl__buildDomainRuntimeType(..., stridable) --> | ||
| // chpl__buildDomainRuntimeType(..., strides) if we are in a DSI class. | ||
| bool replacedStridable(CallExpr* parentCall, const char* name, |
There was a problem hiding this comment.
Calling this function replacedStridable obfuscates the fact that it has a side effect (described in the comment above).
There was a problem hiding this comment.
Renamed to tryReplaceStridable.
compiler/passes/scopeResolve.cpp
Outdated
| if (sym != NULL) { | ||
| if (!replacedStridableSR(name, usymExpr, sym)) | ||
| resolveUnresolvedSymExpr(usymExpr, sym); |
There was a problem hiding this comment.
| if (sym != NULL) { | |
| if (!replacedStridableSR(name, usymExpr, sym)) | |
| resolveUnresolvedSymExpr(usymExpr, sym); | |
| if (sym != NULL) { | |
| if (!replacedStridableSR(name, usymExpr, sym)) | |
| resolveUnresolvedSymExpr(usymExpr, sym); |
| @@ -1265,6 +1272,9 @@ static void checkMethodsOverride() { | |||
| FnSymbol* eFn = getOverrideCandidateGenericFn(fn); | |||
| if (erroredFunctions.count(eFn) == 0) { | |||
| if (fn->hasFlag(FLAG_OVERRIDE)) { | |||
| if (isDsiNewRectangularDom(fn)) { | |||
| // allow, for deprecation by Vass in 1.31 to implement #17131 | |||
There was a problem hiding this comment.
A more informative comment explaining why it's allowed would be nice here: "allowed so that child classes can still override stridable-based versions of the procedure, which is now strides-based". (?)
| return whole[(...(chpl__computeCyclic(this.idxType, locid, dist.targetLocDom.dims(), dist.startIdx)))] | ||
| // supports deprecation by Vass in 1.31 to implement #17131 | ||
| : domain(rank, idxType, stridable=true); |
There was a problem hiding this comment.
Why do we need casts like these?
There was a problem hiding this comment.
The enclosing function has two return statements. The types of their return expressions need to match in order for the function to resolve. This return expression may have a domain type with a more specific value of strides than strideKind.any, whereas the other one always has strideKind.any, translated from stridable=true.
So maybe the right solution is to have the other return expression match the type of this return expression. Since I will be updating Cyclic to get rid of stridable altogether when stridable is deprecated in an upcoming PR, I will do that at that point.
modules/dists/DSIUtil.chpl
Outdated
| @@ -252,7 +266,8 @@ proc computeZeroBasedRanges(ranges: _tuple) { | |||
| // | |||
|
|
|||
| // would like 'whole: domain(?IT,?r,?)' | |||
There was a problem hiding this comment.
Is this still accurate? Particularly ? ?
There was a problem hiding this comment.
What we really would like is whole: rectangularDomain(?). However, this is assumed throughout DSIUtil.chpl, so I am removing this comment altogether.
| @@ -349,7 +366,7 @@ proc densify(sArg: range(?,bounds=?B,stridable=?S), w: range(?IT,?,stridable=fal | |||
|
|
|||
| // gotta have a special case, e.g.: s=1..0 w=5..6 IT=uint | |||
| if isUintType(IT) && s.isEmpty() then | |||
| return 1:IT..0:IT; | |||
| return new range(IT,B,S); | |||
There was a problem hiding this comment.
Why can't we use densiResult here? Does its behavior diverge or is writing range(IT,B,S) just simpler?
There was a problem hiding this comment.
Indeed, in this case densiResult(sArg,w) == range(IT,B,S).
I could add a compilerAssert for this. For now, I just added a comment.
modules/internal/ChapelArray.chpl
Outdated
| compilerError("rectangular domains are not supported by", | ||
| " the distribution ", this.type:string); |
There was a problem hiding this comment.
| compilerError("rectangular domains are not supported by", | |
| " the distribution ", this.type:string); | |
| compilerError("rectangular domains are not supported by", | |
| " the distribution ", this.type:string); |
modules/internal/ChapelDomain.chpl
Outdated
| if ! chpl_assignStrideIsSafe(a.strides, b.strides) then | ||
| compilerError("assigning to a domain with strideKind.",a.strides:string, | ||
| " from a domain with strideKind.", b.strides:string, | ||
| " without an explicit cast"); |
There was a problem hiding this comment.
| if ! chpl_assignStrideIsSafe(a.strides, b.strides) then | |
| compilerError("assigning to a domain with strideKind.",a.strides:string, | |
| " from a domain with strideKind.", b.strides:string, | |
| " without an explicit cast"); | |
| if ! chpl_assignStrideIsSafe(a.strides, b.strides) then | |
| compilerError("assigning to a domain with strideKind.",a.strides:string, | |
| " from a domain with strideKind.", b.strides:string, | |
| " without an explicit cast"); |
modules/internal/ChapelDomain.chpl
Outdated
| for s in chpl__tuplify(this.stride) do | ||
| if s < 0 { | ||
| warning("arrays and array slices with negatively-strided dimensions are currently unsupported and may lead to unexpected behavior; compile with -snoNegativeStrideWarnings to suppress this warning; the dimension(s) are: ", this.dsiDims()); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Indentation is confusing here.
modules/internal/ChapelRange.chpl
Outdated
| } | ||
|
|
||
| @chpldoc.nodoc | ||
| inline proc range.chpl_lastAsIntForIter { | ||
| if bounds == boundKind.both { | ||
| return this.lastAsInt; | ||
| } else { | ||
| if ! stridable { | ||
| if strides == strideKind.one { |
modules/internal/ChapelRange.chpl
Outdated
| @@ -1349,9 +1650,12 @@ operator :(r: range(?), type t: range(?)) { | |||
| HaltWrappers.boundsCheckHalt("indexOrder -- Undefined on a range with ambiguous alignment."); | |||
|
|
|||
| if ! contains(ind) then return (-1):intIdxType; | |||
| if ! stridable { | |||
| if strides == strideKind.one { | |||
There was a problem hiding this comment.
more hasUnitStride opportunities?
modules/internal/ChapelRange.chpl
Outdated
| if ! newStrides.isPosNegOne() { // aka !newStrides.hasParamStride() | ||
| const first = this.orderToIndex(myFollowThis.first); |
| const rStride = ranges(i).stride; | ||
| const rSignedStride = rStride:strType, | ||
| fSignedStride = followThis(i).stride:strType; | ||
| if rStride > 0 { | ||
| if ranges(i).hasPositiveStride() { |
There was a problem hiding this comment.
I use range.hasPositiveStride() because it is a param in some cases, whereas rStride.isPositive() cannot be a param because rStride is an integer and not a param.
| @@ -1,3 +1,4 @@ | |||
| negativeStrideWarnings.chpl:13: warning: when slicing with a range with a negative stride, the sign of the stride of the original range or domain/array dimension is currently preserved, but will be negated in a future release; compile with -snewSliceRule to switch to this new rule and turn off this warning | |||
There was a problem hiding this comment.
Extra warning for the same line -- is this okay?
There was a problem hiding this comment.
Our warnings sometimes do that. I consider it a lower priority to fix this.
vasslitvinov
force-pushed
the
range-stridable
branch
from
b8c3c86
to
17b7403
Compare
Signed-off-by: Vassily Litvinov <vasslitvinov@users.noreply.github.com>
Signed-off-by: Vassily Litvinov <vasslitvinov@users.noreply.github.com>
vasslitvinov
force-pushed
the
range-stridable
branch
from
17b7403
to
91b069e
Compare
Signed-off-by: Vassily Litvinov <vasslitvinov@users.noreply.github.com>
vasslitvinov
force-pushed
the
range-stridable
branch
from
60dce6b
to
6ec17bd
Compare
Signed-off-by: Vassily Litvinov <vasslitvinov@users.noreply.github.com>
#22441 resulted in added `get` communications for the call rhs.getIndices() that it hoisted out from the loop `for e in lhs._arrs` in chpl_assignDomainWithGetSetIndices(). While hoisting helps in many cases, here it was detrimental because this loop had no iterations at all. Given that chpl_assignDomainWithGetSetIndices already invokes rhs.getIndices() outside of the loop, I changed the code to invoke it just once and reuse its result whenever necessary. I also moved the call to castIndices(), which #22441 also hoisted out of the loop, back into the loop. However it probably does not matter because it will be no-op in most cases, otherwise have low overhead. r: @benharsh
#22441 changed how some range bounds are calculated. As a result, the compiler now produces a temp called `call_tmp` where previously it was called `tmp`. This PR adjusts the expectations that the test `anonymousRangeIter` makes about the generated code. Trivial, not reviewed.
This updates the modules to the switch in #22441 from `range.stridable` to `range.strides`. Most of the changes are renamings from `stridable` to `strides` and replacing `anyStridable()` with `chpl_strideUnion()`. However occasionally I did some code reorg and simplification, such as merging two branches of an `if stridable` conditional into code not guarded by a condition. Notably: * Added overloads of `domain.stride` and `domain.alignment` that return a `param` for 1-D rectangular and sparse domains. * Simplified `range.readThis()` and `range.init(..., fileReader, serializer)`; the latter now "throws", rightfully so. * Removed an overload of binarySearch() that could result in an "ambiguous call" error, defeating its purpose of giving a user-friendly error message. * Eliminated distracting differences in where-clauses of fileReader/fileWriter methods in IO.chpl To do post-merge: * enable the deprecation warnings that remain commented out since #22441 * create design issues related to uses of chpl_strideUnion et al., see review comments r: @benharsh
This turns on deprecation warnings for uses of range/domain.stridable introduced in #22441 and switches tests from range/domain.stridable to `strides`. While there: * Fix the bug where `d by (step1:uint, step2:uint)` was not producing a positive-strided domain if `d` was such a domain. * Remove `test/distributions/dm/n.chpl` because it has been unused for a long time. * Remove the future `test/users/vass/crash1callDestructorsMain.chpl` because it no longer causes the internal error that it did eight years ago and bringing it uptodate with all the language changes since then would take a significant amount of time, making it not worth it. I added back the alternative initializer on LocRADCache that I removed in #22486 to support deprecation of user domain maps that happen to use it. r: @jabraham17 @stonea
Update the Ranges and Domains and Coercions chapters of the spec and the comments in the ranges and other primers for the change `range.stridable` --> `strides: strideKind` in #22441, with follow-ons #22486 and #22508. Also remove the remaining occurrences of `BoundedRangeType` from the spec and primers -- it is deprecated as of #22059. Remove the `const` return and yield intent in the Procedures, Iterators, and Tuples sections of the spec -- it is unstable as of #22515. I did not see any need to update the primers for this change. While there, I did some rephrasing and restructuring of the text for better readability or precision. Post-merge: update the Chapel Evolution document. r: @benharsh
#24136) These features were deprecated in 1.31 as follows: range.`boundedType` and `BoundedRangeType` issue: #17126 implementation: #22059 range.`stridable` issue: #17131 implementation: #22441, #22486, #22508 This PR removes support for these features, which was added in the above PRs, and adjusts modules and tests accordingly. More range/domain deprecated feature removals are next steps. r: @jabraham17
This PR switches the boolean field
range.stridabletorange.stridesof the typeenum strideKind { one, negOne, positive, negative, any }according to the design discussion concluded in #17131 (comment)The
byoperator infers thestridesof the resulting range based on itsstepargument. For example,r by 1producesrrather than a range withstridable = true;1..n by anUnsignedIntegerproduces a range withstrideKind.positive; etc.stridablequeries on ranges and domains are still supported without a warning. The warnings have been added to the module and compiler code. In this PR they are commented out and marked with "RSDW" for "range.stridable deprecation warning". They will be enabled in a future PR.I modified Chapel code outside of modules/internal as little as possible. My goal was make sure that the deprecation code works correctly. To support domain maps that have not been converted from stridable to strideKind, the compiler converts accesses to the former field
stridableto the new fieldstridesin BaseRectangularDom and BaseArrOverRectangularDom.The bulk of updates to test code caters for a mix of un-converted uses of
stridablethat produce ranges withstrideKind.anyandstrides-base code produces ranges withstrideKind.positiveor other more specific strideKinds.The bulk of updates to test .good files updates the
stridescomponents of the range and domain types fromfalsetooneand fromtruetopositive,any, etc. Some tests get compile-time warnings or errors instead of, or in addition to, the runtime warnings or errors in those cases where the compiler can now determine the corresponding condition statically based on thestridesparameter of the range or domain.While there:
library/standard/Reflection/primitives/ResolvesDmapif stridableDevelopment history compressed into 36425cd: 42fa8bf..b8c3c86
Post-merge TODOs:
stridesrange.stridesfield inResolver::exit()hasPosNegUnitStrideto a better name