[chapel-lang/chapel] Propagate constness of domain variables into the i

Notifications Commits
1

Branch: refs/heads/master
Revision: 5492b6b
Author: e-kayrakli
Log Message:

Merge pull request #16218 from e-kayrakli/const-domain

Propagate constness of domain variables into the instance implementation

Add a definedConst field in BaseDom and adjust how we create domains to
set that field for const domains. Also, use that information to avoid
tracking arrays for const domains, which is the main motivation for this PR.

Implementation details:

  • Many module helper functions that create domains have a new argument
    definedConst. These helpers include, chpl__initCopy, chpl__autoCopy,
    chpl__coerceCopy and chpl__coerceMove

  • BaseDom has a new field definedConst.

    This field is currently var because there are cases where we need to fix it
    up after initialization. I’ll check if we can handle that case differently
    and can make the field const. See future work for more.

  • Many places in the compiler that create/modify/analyze chpl__initCopy et al
    are adjusted to use the new signature.

  • Coercion helpers (chpl__coerceCopy and chpl__coerceMove) are modified to
    use a new helper to call chpl__convertRuntimeTypeToValue and pass constness
    information to that directly.

  • For optimization: adds a simple check around where we add/remove arrays to the
    domains set of arrays to avoid doing that.

Future work

The implementation was challenging and there are some future steps that we
should take to make the implementation cleaner:

  • See if we can make the definedConst field const or param.

    param-ness makes sense however it has some difficulties as it’ll create a
    completely different type. I am not sure how we can do that without adding
    some notion between const and param for user-defined types that only used
    by the compiler.

    const is probably more doable but there are two major questions that we need
    to address if we want to go in that direction:

    • Can/should we change dsiNewRectangularDomain to accept a new
      definedConst argument? I think the answer to this question should be
      “no”, because we don’t want the implementer of the dmap to deal with
      constness. However, that means that we can’t propogate the information in
      newRectangularDom unless we set it after initialization.

    • Can/should we put the constness information in the privatization data? I
      think the answer is “no” because of similar reasons. Then the question is
      how to propagate the constness information to the privatized instances.
      This PR handles it in ChapelArray after privatized instances are inited.
      Thus, it needs the field to be var.

  • Can we also determine constness by static code analysis even if the domain was
    declared as var?

  • Domain arguments passed to const in formals are not recognized as constant
    within the function body.

Further optimization oppurtunities:

  • Instead of doing lock-increment-unlock, we can use an atomic integer for
    tracking the number of arrays using a domain.

    Arguably an easy change to make. However, will reauire adding one more
    redundant field to the BaseDom class.

  • Can we make use of this information in irregular domains? If we create var
    sparse domain, add indices to it, and then assign it to a const sparse
    domain; can we make that const sparse domain and its arrays work faster in
    some cases?

Other Alternative Implementations Considered

  • We thought about adding a param field to record _domain, but didn’t do it:

    • The information is needed in the domain instance and not in the domain
      record, even though adding a field in the record would be easier.
    • We try to keep the footprint of these records small.

  • Thought about adding “fixups” after all domain initializations, but didn’t do
    it:

    • This would have avoided the need to tinker with chpl__initCopy et al.
      However, it would separate how we set constness from the initialization
      itself. That could cause some issues where we do pattern detection and
      change AST around DefExpr’s.

  • Thought about adding definedConst argument as the first argument and not the
    last.

    There is no ideal choice as to where we add this argument. And I initially
    added it as a first argument. However, there are way too many places in the
    compiler where the 1st and 2nd argument of these functions are used. Whereas,
    the number of places where the arguments are counted from back is very
    limited.

    The only potential confusion with adding this argument as the last is when the
    compiler changes these functions to return via an RVV. In that scenario, the
    definedConst argument is in the middle of arguments. Currently there’s only
    one place in the compiler where this surfaces and that’s in split
    initialization support.

[Reviewed by @mppf]

Test status:

  • [x] standard
  • [x] gasnet

Modified Files:
A test/optimizations/constDomain/arrayOfArrays.chpl
A test/optimizations/constDomain/arrayOfArrays.good
A test/optimizations/constDomain/basic.chpl
A test/optimizations/constDomain/basic.good
A test/optimizations/constDomain/constPrimitiveDecl.chpl
A test/optimizations/constDomain/constPrimitiveDecl.good
A test/optimizations/constDomain/ifExpr.chpl
A test/optimizations/constDomain/ifExpr.good
A test/optimizations/constDomain/inIntents.bad
A test/optimizations/constDomain/inIntents.chpl
A test/optimizations/constDomain/inIntents.future
A test/optimizations/constDomain/inIntents.good
A test/optimizations/constDomain/lowLevel.chpl
A test/optimizations/constDomain/lowLevel.good
A test/optimizations/constDomain/privatization.chpl
A test/optimizations/constDomain/privatization.good
A test/optimizations/constDomain/privatization.numlocales
A test/optimizations/constDomain/privatization.skipif
M compiler/AST/AggregateType.cpp
M compiler/AST/AstToText.cpp
M compiler/AST/build.cpp
M compiler/include/bison-chapel.h
M compiler/include/resolution.h
M compiler/main/checks.cpp
M compiler/optimizations/removeUnnecessaryAutoCopyCalls.cpp
M compiler/parser/bison-chapel.cpp
M compiler/parser/chapel.ypp
M compiler/passes/errorHandling.cpp
M compiler/passes/parallel.cpp
M compiler/passes/splitInit.cpp
M compiler/resolution/callDestructors.cpp
M compiler/resolution/functionResolution.cpp
M compiler/resolution/initializerResolution.cpp
M compiler/resolution/lowerIterators.cpp
M compiler/resolution/preFold.cpp
M compiler/resolution/resolveFunction.cpp
M compiler/resolution/tuples.cpp
M compiler/resolution/wrappers.cpp
M modules/internal/ArrayViewRankChange.chpl
M modules/internal/ByteBufferHelpers.chpl
M modules/internal/CString.chpl
M modules/internal/ChapelArray.chpl
M modules/internal/ChapelBase.chpl
M modules/internal/ChapelDistribution.chpl
M modules/internal/ChapelIteratorSupport.chpl
M modules/internal/ChapelSyncvar.chpl
M modules/internal/DefaultRectangular.chpl
M modules/internal/LocaleModelHelpRuntime.chpl
M modules/internal/OwnedObject.chpl
M modules/packages/Futures.chpl
M test/arrays/slices/commCounts/sliceBlockWithRanges.na-none.good
M test/distributions/bradc/wrongDomForDist/DummyArithDist.chpl
M test/distributions/robust/arithmetic/performance/multilocale/assignReindex.block.good
M test/distributions/robust/arithmetic/performance/multilocale/assignReindex.block.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/assignReindex.cyclic.good
M test/distributions/robust/arithmetic/performance/multilocale/assignReindex.cyclic.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/assignReindex.replicated.good
M test/distributions/robust/arithmetic/performance/multilocale/assignReindex.replicated.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/assignSlice.block.good
M test/distributions/robust/arithmetic/performance/multilocale/assignSlice.block.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/assignSlice.cyclic.good
M test/distributions/robust/arithmetic/performance/multilocale/assignSlice.cyclic.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/assignSlice.replicated.good
M test/distributions/robust/arithmetic/performance/multilocale/assignSlice.replicated.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/reduceSlice.block.good
M test/distributions/robust/arithmetic/performance/multilocale/reduceSlice.block.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/reduceSlice.cyclic.good
M test/distributions/robust/arithmetic/performance/multilocale/reduceSlice.cyclic.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/reduceSlice.replicated.good
M test/distributions/robust/arithmetic/performance/multilocale/reduceSlice.replicated.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignReindex.block.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignReindex.block.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignReindex.cyclic.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignReindex.cyclic.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignReindex.replicated.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignReindex.replicated.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignSlice.block.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignSlice.block.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignSlice.cyclic.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignSlice.cyclic.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignSlice.replicated.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/assignSlice.replicated.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/reduceSlice.block.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/reduceSlice.block.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/reduceSlice.cyclic.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/reduceSlice.cyclic.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/reduceSlice.replicated.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/reduceSlice.replicated.na-none.good
M test/distributions/robust/arithmetic/performance/multilocale/rvfSlices/reduceSlice2.good
M test/functions/iterators/ferguson/no-leak-record-bug.chpl
M test/functions/iterators/ferguson/no-leak-yield-record.chpl
M test/types/records/ferguson/dynamic-dispatch-autocopy.chpl
M test/types/records/ferguson/missing-auto-copy.chpl

Compare: https://github.com/chapel-lang/chapel/compare/d59235aaca86...5492b6b941c4