Skip to content

Commit

Permalink
Add opt-in mode to resolve concrete functions (chapel-lang#24036)
Browse files Browse the repository at this point in the history
When generating a .dyno file, we need to resolve and code-generate the
concrete functions for separate compilation to make sense.

Additionally, it is sometimes useful to resolve concrete functions as a
way to ensuring the quality of one's library.

So, this PR adds an experimental opt-in flag `--resolve-concrete-fns` to
enable resolving concrete functions. Having this behavior be on by
default is discussed in issue chapel-lang#10276 but I don't need it to be
on-by-default for my purposes. The implementation is based upon earlier
work by @bradcray in the branch from PR chapel-lang#10449.

Some functions are overloads that exist to give an error message; e.g. a
function containing only a `compilerError` call. I think that the ideal
case for such functions would be for the compiler to resolve them but
still produce the error message at call sites. This will require some
attention with the separate compilation. However, such behavior is
difficult to implement with the production resolver and I opted to leave
it alone for now & I plan to revisit when we have the new resolver
online.

So, this PR uses a workaround for such functions by marking them with a
new pragma to disable their resolution under `--resolve-concrete-fns`.
This is also a difference from @bradcray's previous effort, which
resolved such functions but instructed the compiler to hide the errors
and warnings. I am concerned that this hiding strategy would hide real
errors.

Additionally, this PR removes a few deprecated features rather than
updating them with the pragma. It removes `CHPL_AUX_FILESYS` and
`file.localesForRegion`.

Lastly, this PR updates some module code to address bugs that were
revealed by compiling simple programs with `--resolve-concrete-fns`.

Reviewed by @DanilaFe - thanks!

- [x] full comm=none testing
  • Loading branch information
mppf authored Dec 12, 2023
2 parents e6f0a6d + 5040e43 commit 8d3a5e2
Show file tree
Hide file tree
Showing 33 changed files with 169 additions and 116 deletions.
2 changes: 2 additions & 0 deletions compiler/include/driver.h
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,8 @@ extern bool fDynoVerifySerialization;

extern size_t fDynoBreakOnHash;

extern bool fResolveConcreteFns;

extern bool fNoIOGenSerialization;
extern bool fNoIOSerializeWriteThis;
extern bool fNoIODeserializeReadThis;
Expand Down
3 changes: 3 additions & 0 deletions compiler/main/driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,8 @@ bool fDynoDebugTrace = false;
bool fDynoVerifySerialization = false;
size_t fDynoBreakOnHash = 0;

bool fResolveConcreteFns = false;

bool fNoIOGenSerialization = false;
bool fNoIOSerializeWriteThis = false;
bool fNoIODeserializeReadThis = false;
Expand Down Expand Up @@ -1488,6 +1490,7 @@ static ArgumentDescription arg_desc[] = {
{"dyno-break-on-hash", ' ' , NULL, "Break when query with given hash value is executed when using dyno compiler library", "X", &fDynoBreakOnHash, "CHPL_DYNO_BREAK_ON_HASH", NULL},
{"dyno-gen-lib", ' ', "<path>", "Specify files named on the command line should be saved into a .dyno library", "P", NULL, NULL, addDynoGenLib},
{"dyno-verify-serialization", ' ', NULL, "Enable [disable] verification of serialization", "N", &fDynoVerifySerialization, NULL, NULL},
{"resolve-concrete-fns", ' ', NULL, "Enable [disable] resolving concrete functions", "N", &fResolveConcreteFns, NULL, NULL},
{"foreach-intents", ' ', NULL, "Enable [disable] (current, experimental, support for) foreach intents.", "N", &fForeachIntents, "CHPL_FOREACH_INTENTS", NULL},

{"io-gen-serialization", ' ', NULL, "Enable [disable] generation of IO serialization methods", "n", &fNoIOGenSerialization, "CHPL_IO_GEN_SERIALIZATION", NULL},
Expand Down
65 changes: 62 additions & 3 deletions compiler/resolution/functionResolution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ static Expr* foldTryCond(Expr* expr);
static void unmarkDefaultedGenerics();
static void resolveUsesAndModule(ModuleSymbol* mod, const char* path);
static void resolveSupportForModuleDeinits();
static void resolveExports();
static void resolveExportsEtc();
static void resolveEnumTypes();
static void populateRuntimeTypeMap();
static void resolveAutoCopies();
Expand Down Expand Up @@ -11436,7 +11436,7 @@ void resolve() {

finishInterfaceChecking(); // should happen before resolveAutoCopies

resolveExports();
resolveExportsEtc();

resolveEnumTypes();

Expand Down Expand Up @@ -11623,9 +11623,30 @@ static void resolveSupportForModuleDeinits() {
* *
************************************** | *************************************/

static void resolveExports() {
static bool hasVariableArgs(FnSymbol* fn) {
for_formals(formal, fn) {
if (formal->variableExpr) {
return true;
}
}

return false;
}

static bool isGenericFn(FnSymbol* fn) {
if (!fn->isGenericIsValid()) {
fn->tagIfGeneric();
}
return fn->isGeneric();
}

static void resolveExportsEtc() {
std::vector<FnSymbol*> exps;

// try to resolve concrete functions when using --dyno-gen-lib
bool alsoConcrete = (fResolveConcreteFns || !gDynoGenLibOutput.empty()) &&
!fMinimalModules;

// We need to resolve any additional functions that will be exported.
forv_expanding_Vec(FnSymbol, fn, gFnSymbols) {
if (fn == initStringLiterals) {
Expand All @@ -11642,6 +11663,44 @@ static void resolveExports() {

if (fn->hasFlag(FLAG_EXPORT))
exps.push_back(fn);
} else if (alsoConcrete) {
// gather the receiver type if there is one
AggregateType* at = NULL;
if (fn->_this) {
at = toAggregateType(fn->_this->type);
}

if (!fn->hasFlag(FLAG_GENERIC) &&
!fn->hasFlag(FLAG_LAST_RESORT) /* often a compilerError overload*/ &&
!fn->hasFlag(FLAG_DO_NOT_RESOLVE_UNLESS_CALLED) &&
!hasVariableArgs(fn) &&
!fn->hasFlag(FLAG_RESOLVED) &&
!fn->hasFlag(FLAG_INVISIBLE_FN) &&
!fn->hasFlag(FLAG_INLINE) &&
!fn->hasFlag(FLAG_EXTERN) &&
!fn->hasFlag(FLAG_ON) &&
!fn->hasFlag(FLAG_COBEGIN_OR_COFORALL) &&
!fn->hasFlag(FLAG_COMPILER_GENERATED) &&
// either this is not a method, or at least it's not a method
// on a generic type
(fn->_this == NULL || !at || !at->isGeneric()) &&
// for now, ignore chpl_ functions
(strncmp(fn->name, "chpl_", 5) != 0) &&
fn->defPoint &&
// Nested functions are tricky because their resolution may depend
// on the resolution of the outer function in which they are located;
// i.e., they may be generic w.r.t. outer-scoped variables, yet not
// marked with FLAG_GENERIC. For now, rule out all nested functions.
!isFnSymbol(fn->defPoint->parentSymbol) && // fn is not nested
fn->defPoint->getModule() &&
!isGenericFn(fn)
) {
SET_LINENO(fn);

if (evaluateWhereClause(fn)) {
resolveSignatureAndFunction(fn);
}
}
}
}

Expand Down
4 changes: 4 additions & 0 deletions compiler/resolution/resolveFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,10 @@ void resolveFunction(FnSymbol* fn, CallExpr* forCall) {

insertUnrefForArrayOrTupleReturn(fn);

if (fn->retExprType) {
resolveSpecifiedReturnType(fn);
}

Type* yieldedType = NULL;
resolveReturnTypeAndYieldedType(fn, &yieldedType);

Expand Down
1 change: 1 addition & 0 deletions frontend/include/chpl/uast/PragmaList.h
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,7 @@ PRAGMA(EXEMPT_INSTANTIATION_LIMIT, ypr, "fn exempt instantiation limit", "compil

PRAGMA(COMPUTE_UNIFIED_TYPE_HELP, ypr, "compute unified type helper", "identify the internal chpl_computeUnifiedTypeHelp() routine")

PRAGMA(DO_NOT_RESOLVE_UNLESS_CALLED, npr, "do not resolve unless called", "do not resolve this function unless it is called (e.g. if it contains only compilerError)")
#undef ypr
#undef npr
#undef ncm
1 change: 1 addition & 0 deletions modules/internal/Atomics.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ module Atomics {
}

// TODO: should this be an operator method AtomicBool.: ?
pragma "do not resolve unless called"
@chpldoc.nodoc
operator :(rhs: bool, type t:AtomicBool) {
var lhs: AtomicBool = rhs; // use init=
Expand Down
2 changes: 1 addition & 1 deletion modules/internal/BytesStringCommon.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,7 @@ module BytesStringCommon {

if curMargin == '':t {
// An unindented non-empty line means no margin exists, return early
margin = '';
margin = '':t;
break;
} else if margin == '':t {
// Initialize margin
Expand Down
2 changes: 2 additions & 0 deletions modules/internal/ChapelBase.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,13 @@ module ChapelBase {
inline operator c_fn_ptr.=(ref a:c_fn_ptr, b:c_fn_ptr) {
__primitive("=", a, b);
}
pragma "do not resolve unless called"
@chpldoc.nodoc
@unstable
proc c_fn_ptr.this() {
compilerError("Can't call a C function pointer within Chapel");
}
pragma "do not resolve unless called"
@chpldoc.nodoc
@unstable
proc c_fn_ptr.this(args...) {
Expand Down
4 changes: 2 additions & 2 deletions modules/internal/ChapelLocale.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -558,12 +558,12 @@ module ChapelLocale {
// LocaleSpace -- an array of locales and its corresponding domain
// which are used as the default set of targetLocales in many
// distributions.
proc getDefaultLocaleSpace() const ref {
proc getDefaultLocaleSpace() const ref : chpl_emptyLocaleSpace.type {
HaltWrappers.pureVirtualMethodHalt();
return chpl_emptyLocaleSpace;
}

proc getDefaultLocaleArray() const ref {
proc getDefaultLocaleArray() const ref : chpl_emptyLocales.type {
HaltWrappers.pureVirtualMethodHalt();
return chpl_emptyLocales;
}
Expand Down
2 changes: 1 addition & 1 deletion modules/internal/DefaultRectangular.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ module DefaultRectangular {
proc dsiEqualDMaps(d:unmanaged DefaultDist) param do return true;
proc dsiEqualDMaps(d) param do return false;

proc trackDomains() param do return false;
override proc trackDomains() param do return false;
override proc dsiTrackDomains() do return false;

override proc singleton() param do return true;
Expand Down
6 changes: 6 additions & 0 deletions modules/internal/LocaleModelHelpSetup.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ module LocaleModelHelpSetup {
}

proc helpSetupRootLocaleNUMA(dst:borrowed RootLocale) {
extern proc chpl_task_setSubloc(subloc: int(32));

var root_accum:chpl_root_locale_accum;

forall locIdx in dst.chpl_initOnLocales() with (ref root_accum) {
Expand All @@ -105,6 +107,8 @@ module LocaleModelHelpSetup {
}

proc helpSetupRootLocaleAPU(dst:borrowed RootLocale) {
extern proc chpl_task_setSubloc(subloc: int(32));

var root_accum:chpl_root_locale_accum;

forall locIdx in dst.chpl_initOnLocales() with (ref root_accum) {
Expand All @@ -119,6 +123,8 @@ module LocaleModelHelpSetup {
}

proc helpSetupRootLocaleGPU(dst:borrowed RootLocale) {
extern proc chpl_task_setSubloc(subloc: int(32));

var root_accum:chpl_root_locale_accum;

forall locIdx in dst.chpl_initOnLocales() with (ref root_accum) {
Expand Down
1 change: 1 addition & 0 deletions modules/internal/NetworkAtomics.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ module NetworkAtomics {

}

pragma "do not resolve unless called"
operator :(rhs: bool, type t:RAtomicBool) {
var lhs: RAtomicBool = rhs; // use init=
return lhs;
Expand Down
8 changes: 0 additions & 8 deletions modules/standard/ChplConfig.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,6 @@ module ChplConfig {
param CHPL_HOME:string;
CHPL_HOME = __primitive("get compiler variable", "CHPL_HOME");

/* Deprecated */
@deprecated(notes="CHPL_AUX_FILESYS is deprecated, please let us know if this is a problem")
proc CHPL_AUX_FILESYS param :string {
// use a proc here because the split initialization caused an
// additional deprecation warning
return __primitive("get compiler variable", "CHPL_AUX_FILESYS");
}

/* See :ref:`readme-chplenv.CHPL_TARGET_PLATFORM` for more information. */
@unstable("'ChplConfig.CHPL_TARGET_PLATFORM' is unstable and may be replaced with a different way to access this information in the future");
param CHPL_TARGET_PLATFORM:string;
Expand Down
56 changes: 3 additions & 53 deletions modules/standard/IO.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -2051,8 +2051,9 @@ proc file.unlock() {
*/

@chpldoc.nodoc
proc file.filePlugin() : QioPluginFile? {
return qio_file_get_plugin(this._channel_internal);
proc file.filePlugin() : borrowed QioPluginFile? {
var vptr = qio_file_get_plugin(this._file_internal);
return vptr:borrowed QioPluginFile?;
}

// File style cannot be modified after the file is created;
Expand Down Expand Up @@ -9935,57 +9936,6 @@ proc file.fstype():int throws {
return t:int;
}

/*
Returns the 'best' locale to run something working with the region
of the file in start..end-1.
This *must* return the same result when called from different locales.
Returns a domain of locales that are "best" for the given region. If no
locales are "best" we return a domain containing all locales.
:arg start: the file offset (starting from 0) where the region begins
:arg end: the file offset just after the region
:returns: a set of locales that are best for working with this region
:rtype: domain(locale)
*/
@deprecated(notes="file.localesForRegion is deprecated")
proc file.localesForRegion(start:int(64), end:int(64)) {

proc findloc(loc:string, locs:c_ptr(c_ptrConst(c_char)), end:int) {
for i in 0..end-1 {
if (loc == locs[i]) then
return true;
}
return false;
}

var ret: domain(locale);
on this._home {
var err:errorCode;
var locs: c_ptr(c_ptrConst(c_char));
var num_hosts:c_int;
err = qio_locales_for_region(this._file_internal, start, end, c_ptrTo(locs), num_hosts);
// looping over Locales enforces the ordering constraint on the locales.
for loc in Locales {
if (findloc(loc.name, locs, num_hosts:int)) then
ret += loc;
}

// We allocated memory in the runtime for this, so free it now
if num_hosts != 0 {
for i in 0..num_hosts-1 do
deallocate(locs[i]);
deallocate(locs);
}

// We found no "good" locales. So any locale is just as good as the next
if ret.size == 0 then
for loc in Locales do
ret += loc;
}
return ret;
}


/*
Expand Down
1 change: 1 addition & 0 deletions modules/standard/Regex.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@ record regexMatch {
var numBytes:int;
}

pragma "do not resolve unless called"
@chpldoc.nodoc
proc reMatch type
{
Expand Down
13 changes: 8 additions & 5 deletions modules/standard/Time.chpl
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ module Time {
/* Specifies the day of the week */
@deprecated(notes="enum 'day' is deprecated. Please use :enum:`dayOfWeek` instead")
enum day { sunday=0, monday, tuesday, wednesday, thursday, friday, saturday }
pragma "do not resolve unless called"
@chpldoc.nodoc
proc DayOfWeek {
compilerError("'DayOfWeek' was renamed. Please use 'dayOfWeek' instead");
Expand All @@ -163,6 +164,7 @@ module Time {
Saturday = 6,
Sunday = 7
}
pragma "do not resolve unless called"
@chpldoc.nodoc
proc ISODayOfWeek {
compilerError("'ISODayOfWeek was renamed. Please use 'isoDayOfWeek' instead");
Expand Down Expand Up @@ -2105,7 +2107,7 @@ module Time {
}

@chpldoc.nodoc
operator timeDelta.>(lhs: timeDelta, rhs: timeDelta) : timeDelta {
operator timeDelta.>(lhs: timeDelta, rhs: timeDelta) : bool {
const ls = (lhs.days*(24*60*60) + lhs.seconds);
const rs = (rhs.days*(24*60*60) + rhs.seconds);
if ls > rs then return true;
Expand Down Expand Up @@ -2152,6 +2154,7 @@ module Time {

@chpldoc.nodoc
operator :(t: timeDelta, type s:string) : string {
import Math;
var str: string;
if t.days != 0 {
str = t.days: string + " day";
Expand All @@ -2167,13 +2170,13 @@ module Time {
str += hours: string + ":";
if minutes < 10 then
str += "0";
str += minutes + ":";
str += minutes:string + ":";
if seconds < 10 then
str += "0";
str += seconds;
str += seconds:string;
if microseconds != 0 {
str += ".";
const usLog10 = log10(microseconds): int;
const usLog10 = Math.log10(microseconds): int;
for i in 1..(5-usLog10) {
str += "0";
}
Expand Down Expand Up @@ -2425,7 +2428,7 @@ record Timer {
Clears the elapsed time. If the timer is running then it is restarted
otherwise it remains in the stopped state.
*/
proc clear() : void {
proc ref clear() : void {
accumulated = 0.0;

if running {
Expand Down
4 changes: 4 additions & 0 deletions test/compflags/ferguson/resolve-concrete-fns.chpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
proc foo() {
var x: int = "hello";
return 1;
}
1 change: 1 addition & 0 deletions test/compflags/ferguson/resolve-concrete-fns.compopts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
--resolve-concrete-fns
2 changes: 2 additions & 0 deletions test/compflags/ferguson/resolve-concrete-fns.good
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
resolve-concrete-fns.chpl:1: In function 'foo':
resolve-concrete-fns.chpl:2: error: cannot initialize 'x' of type 'int(64)' from '"hello"'
7 changes: 0 additions & 7 deletions test/deprecated/IO/localesForRegion.chpl

This file was deleted.

Loading

0 comments on commit 8d3a5e2

Please sign in to comment.