From 39f5445798a9104a535a01433039dcd86bf7d3c5 Mon Sep 17 00:00:00 2001 From: Vassily Litvinov Date: Thu, 8 Feb 2024 11:38:16 -0800 Subject: [PATCH 1/4] Denormalize return by ref Signed-off-by: Vassily Litvinov --- compiler/passes/denormalize.cpp | 199 ++++++++++++++++++++ compiler/resolution/interfaceResolution.cpp | 1 + 2 files changed, 200 insertions(+) diff --git a/compiler/passes/denormalize.cpp b/compiler/passes/denormalize.cpp index a22813eb1131..4eb3cbce09ab 100644 --- a/compiler/passes/denormalize.cpp +++ b/compiler/passes/denormalize.cpp @@ -37,6 +37,8 @@ #include "global-ast-vecs.h" +static void undoReturnByRef(FnSymbol* fn); + //helper datastructures/types typedef std::pair DefCastPair; typedef std::map UseDefCastMap; @@ -89,6 +91,7 @@ void denormalize(void) { forv_Vec(FnSymbol, fn, gFnSymbols) { // remove unused epilogue labels removeUnnecessaryGotos(fn, true); + if (fn->hasFlag(FLAG_FN_RETARG)) undoReturnByRef(fn); bool isFirstRound = true; do { @@ -669,3 +672,199 @@ inline bool unsafeExprInBetween(Expr* e1, Expr* e2, Expr* exprToMove, } return false; } + +///////////////////////////////////////////////////////////////////////////// +// undoReturnByRef() +// +// Ultimately we want to avoid introducing return by ref. +// For now, simply undo it as a denormalization step. + +/////////// +// +// transformRetTempDef() will modify 'fn' to replace +// =(refArg, something) +// return(void) +// with: +// return(something) + +class DefInfoRR { +public: + FnSymbol* fn; // the function that returns by ref + ArgSymbol* refArg; // the function's formal implementing that + bool skipLnFnArgs; // if true, refArg is fn's 3rd-last formal +}; + +static bool isGoodRefArg(FnSymbol* fn, DefInfoRR& info, + bool skippedLnFnArgs, Expr* argDef) { + ArgSymbol* refArg = toArgSymbol(toDefExpr(argDef)->sym); + if (!refArg->hasFlag(FLAG_RETARG)) + return false; // give up; maybe look harder? + INT_ASSERT(!strcmp(refArg->name, "_retArg")); + INT_ASSERT(fn->retType = dtVoid); + + info = { fn, refArg, skippedLnFnArgs }; + return true; +} + +static bool acceptableDef(FnSymbol* fn, DefInfoRR& info) { + // isGoodRefArg() fills 'info' + if (! isGoodRefArg(fn, info, false, fn->formals.tail) && + ! (fn->numFormals() >= 3 && + isGoodRefArg(fn, info, true, fn->formals.tail->prev->prev)) ) + return false; + + if (info.refArg->type->symbol->hasFlag(FLAG_STAR_TUPLE)) + return false; // codegen requires star tuples to be passed by ref + + for_SymbolSymExprs(refSE, info.refArg) { + if (CallExpr* call = toCallExpr(refSE->parentExpr)) { + if (call->isPrimitive(PRIM_NO_ALIAS_SET)) + continue; // any of these are ok + if (! call->isPrimitive(PRIM_ASSIGN)) + return false; // need more work to handle, ex., PRIM_SET_MEMBER + // transformRetTempDef() will assert that we have only one PRIM_ASSIGN + } + } + + return true; +} + +static void transformRetTempDef(DefInfoRR& info) { + SymExpr* refUse = nullptr; + for_SymbolSymExprs(refSE, info.refArg) { + if (CallExpr* call = toCallExpr(refSE->parentExpr)) + if (call->isPrimitive(PRIM_NO_ALIAS_SET)) { + if (call->numActuals() == 1) call->remove(); + else refSE->remove(); + continue; + } + INT_ASSERT(refUse == nullptr); // expect only a single SE + refUse = refSE; + } + + CallExpr* assignCall = toCallExpr(refUse->parentExpr); + Expr* refValueExpr = assignCall->get(2)->remove(); + INT_ASSERT(isSymExpr(refValueExpr)); // ensure it can be used in 'return' + // Do we need to add FLAG_RVV to refValueExpr->symbol()? + // At this point temps with FLAG_RVV may occur anywhere due to inlining. + assignCall->remove(); + + CallExpr* returnCall = toCallExpr(info.fn->body->body.tail); + INT_ASSERT(returnCall->isPrimitive(PRIM_RETURN)); + INT_ASSERT(toSymExpr(returnCall->get(1))->symbol() == gVoid); + returnCall->get(1)->replace(refValueExpr); + + info.refArg->defPoint->remove(); + info.fn->retType = info.refArg->type; + info.fn->removeFlag(FLAG_FN_RETARG); +} + +/////////// +// +// transformRetTempUse() will replace: +// def ret_tmp +// call fn(..., ret_tmp) +// move(..., ret_tmp) +// with: +// move(..., call fn(...)) + +class UseInfoRR { +public: + SymExpr* fnSE; // the SymExpr for 'fn' in 'call fn(..., ret_tmp)' above + SymExpr* argToFn; // the SymExpr for 'ret_tmp' in 'fn(..., ret_tmp)' above + SymExpr* argToMove; // the SymExpr for 'ret_tmp' in 'move' above; +}; // argToMove is NULL when no 'move' ie ret_tmp is unused + +// Return true if the desired pattern is present. +static bool acceptableUse(DefInfoRR& defInfo, SymExpr* fnUse, + UseInfoRR& useInfo) { + CallExpr* call = toCallExpr(fnUse->parentExpr); + if (call == nullptr || call->resolvedFunction() != defInfo.fn) + return false; + + INT_ASSERT(call == call->getStmtExpr()); + SymExpr* lastActual = toSymExpr(defInfo.skipLnFnArgs ? + call->argList.tail->prev->prev : call->argList.tail); + Symbol* retTemp = lastActual->symbol(); + // retTemp is usually called "ret_tmp", however not necessarily + INT_ASSERT(retTemp->type == defInfo.refArg->type); // fyi + + SymExpr* tempRead = nullptr; + for_SymbolSymExprs(tempUse, retTemp) { + if (tempUse == lastActual) continue; + CallExpr* move = toCallExpr(tempUse->parentExpr); + // is PRIM_ASSIGN also OK? + if (move == nullptr || ! move->isPrimitive(PRIM_MOVE)) + return false; + INT_ASSERT(tempRead == nullptr); // expect only a single use + tempRead = tempUse; + } + + useInfo = { fnUse, lastActual, tempRead }; + return true; +} + +static void transformRetTempUse(UseInfoRR& info) { + Symbol* retTemp = info.argToFn->symbol(); + info.argToFn->remove(); + //fyi: + INT_ASSERT(! retTemp->type->symbol->hasEitherFlag(FLAG_REF, FLAG_WIDE_REF)); + Expr* fnCall = info.fnSE->parentExpr; + + if (retTemp->isRef()) { + // replace: + // call fn(args, tmp) + // with: + // def call_tmp + // move(call_tmp, call fn(args)) + // assign(tmp, call_tmp) + SET_LINENO(fnCall); + VarSymbol* callTemp = newTempConst("call_temp", retTemp->type); + DefExpr* ctDef = new DefExpr(callTemp); + CallExpr* move = new CallExpr(PRIM_MOVE, callTemp); + CallExpr* assign = new CallExpr(PRIM_ASSIGN, retTemp, callTemp); + fnCall->insertBefore(ctDef); + fnCall->insertBefore(move); + fnCall->insertBefore(assign); + move->insertAtTail(fnCall->remove()); + } + else { + // replace: + // def ret_tmp + // call fn(args, ret_tmp) + // move(target, ret_tmp + // with: + // move(target, call fn(args)) + if (info.argToMove == nullptr) + return; + info.argToMove->replace(fnCall->remove()); + retTemp->defPoint->remove(); + } +} + +/////////// +// put it all together + +static void undoReturnByRef(FnSymbol* fn) { + if (fn->hasFlag(FLAG_VIRTUAL)) return; // skip these for now + + DefInfoRR defInfo; + if (! acceptableDef(fn, defInfo)) + return; + + // Make changes only if we can handle all uses of 'fn'. + // While checking, store some findings for later use. + std::vector infos; + for_SymbolSymExprs(use, fn) { + UseInfoRR useInfo; + if (acceptableUse(defInfo, use, useInfo)) + infos.push_back(useInfo); + else + return; + } + + for (UseInfoRR& info: infos) + transformRetTempUse(info); + + transformRetTempDef(defInfo); +} diff --git a/compiler/resolution/interfaceResolution.cpp b/compiler/resolution/interfaceResolution.cpp index 0d078ee28ded..46c3ef4ef05b 100644 --- a/compiler/resolution/interfaceResolution.cpp +++ b/compiler/resolution/interfaceResolution.cpp @@ -1334,6 +1334,7 @@ static FnSymbol* finalizeHolder(ImplementsStmt* istm, FnSymbol* reqFn, wrapper->insertAtTail("'return'(%S)", gVoid); } else { VarSymbol* retTemp = newTemp("ret", wrapper->retType); + retTemp->addFlag(FLAG_RVV); // If this is violated, need to handle that case. INT_ASSERT(call->parentExpr == wrapper->body); call->insertBefore(new DefExpr(retTemp)); From d0343203333e45cbadb27ca3cc26578c446a7e62 Mon Sep 17 00:00:00 2001 From: Vassily Litvinov Date: Thu, 8 Feb 2024 18:04:29 -0800 Subject: [PATCH 2/4] Add removeTrivialMoves() Signed-off-by: Vassily Litvinov --- compiler/passes/denormalize.cpp | 178 +++++++++++++++++++++----------- 1 file changed, 119 insertions(+), 59 deletions(-) diff --git a/compiler/passes/denormalize.cpp b/compiler/passes/denormalize.cpp index 4eb3cbce09ab..bea4a610bcd3 100644 --- a/compiler/passes/denormalize.cpp +++ b/compiler/passes/denormalize.cpp @@ -38,6 +38,7 @@ #include "global-ast-vecs.h" static void undoReturnByRef(FnSymbol* fn); +static void collapseTrivialMoves(); //helper datastructures/types typedef std::pair DefCastPair; @@ -115,6 +116,8 @@ void denormalize(void) { isFirstRound = false; } while(deferredSyms.size() > 0); } + + collapseTrivialMoves(); } } @@ -762,18 +765,17 @@ static void transformRetTempDef(DefInfoRR& info) { /////////// // // transformRetTempUse() will replace: -// def ret_tmp -// call fn(..., ret_tmp) -// move(..., ret_tmp) +// call fn(args, ret_tmp) // with: -// move(..., call fn(...)) +// move(ret_tmp, call fn(args)) +// +// collapseTrivialMoves() will reduce to the original 'call_temp = fn(...)' class UseInfoRR { -public: - SymExpr* fnSE; // the SymExpr for 'fn' in 'call fn(..., ret_tmp)' above - SymExpr* argToFn; // the SymExpr for 'ret_tmp' in 'fn(..., ret_tmp)' above - SymExpr* argToMove; // the SymExpr for 'ret_tmp' in 'move' above; -}; // argToMove is NULL when no 'move' ie ret_tmp is unused +public: // in 'call fn(args, ret_tmp)' above, the SymExprs for: + SymExpr* fnSE; // 'fn' + SymExpr* tempSE; // 'ret_tmp' +}; // Return true if the desired pattern is present. static bool acceptableUse(DefInfoRR& defInfo, SymExpr* fnUse, @@ -783,67 +785,38 @@ static bool acceptableUse(DefInfoRR& defInfo, SymExpr* fnUse, return false; INT_ASSERT(call == call->getStmtExpr()); - SymExpr* lastActual = toSymExpr(defInfo.skipLnFnArgs ? + SymExpr* tempSE = toSymExpr(defInfo.skipLnFnArgs ? call->argList.tail->prev->prev : call->argList.tail); - Symbol* retTemp = lastActual->symbol(); - // retTemp is usually called "ret_tmp", however not necessarily - INT_ASSERT(retTemp->type == defInfo.refArg->type); // fyi - - SymExpr* tempRead = nullptr; - for_SymbolSymExprs(tempUse, retTemp) { - if (tempUse == lastActual) continue; - CallExpr* move = toCallExpr(tempUse->parentExpr); - // is PRIM_ASSIGN also OK? - if (move == nullptr || ! move->isPrimitive(PRIM_MOVE)) - return false; - INT_ASSERT(tempRead == nullptr); // expect only a single use - tempRead = tempUse; - } + // the temp is usually called "ret_tmp", however not necessarily + INT_ASSERT(tempSE->symbol()->type == defInfo.refArg->type); // fyi - useInfo = { fnUse, lastActual, tempRead }; + useInfo = { fnUse, tempSE }; return true; } static void transformRetTempUse(UseInfoRR& info) { - Symbol* retTemp = info.argToFn->symbol(); - info.argToFn->remove(); + Expr* fnCall = info.fnSE->parentExpr; + Symbol* retTemp = info.tempSE->symbol(); //fyi: INT_ASSERT(! retTemp->type->symbol->hasEitherFlag(FLAG_REF, FLAG_WIDE_REF)); - Expr* fnCall = info.fnSE->parentExpr; - - if (retTemp->isRef()) { - // replace: - // call fn(args, tmp) - // with: - // def call_tmp - // move(call_tmp, call fn(args)) - // assign(tmp, call_tmp) - SET_LINENO(fnCall); - VarSymbol* callTemp = newTempConst("call_temp", retTemp->type); - DefExpr* ctDef = new DefExpr(callTemp); - CallExpr* move = new CallExpr(PRIM_MOVE, callTemp); - CallExpr* assign = new CallExpr(PRIM_ASSIGN, retTemp, callTemp); - fnCall->insertBefore(ctDef); - fnCall->insertBefore(move); - fnCall->insertBefore(assign); - move->insertAtTail(fnCall->remove()); - } - else { - // replace: - // def ret_tmp - // call fn(args, ret_tmp) - // move(target, ret_tmp - // with: - // move(target, call fn(args)) - if (info.argToMove == nullptr) - return; - info.argToMove->replace(fnCall->remove()); - retTemp->defPoint->remove(); - } + + // replace: + // call fn(args, ret_tmp) + // with: + // move(ret_tmp, call fn(args)) // if ret_tmp is a ref, make it assign + // + SET_LINENO(fnCall); + + Expr* anchor = fnCall->prev; + BlockStmt* encl = anchor ? NULL : toBlockStmt(fnCall->parentExpr); + Expr* move = new CallExpr(retTemp->isRef() ? PRIM_ASSIGN : PRIM_MOVE, + info.tempSE->remove(), fnCall->remove()); + if (anchor) anchor->insertAfter(move); + else encl->insertAtHead(move); } /////////// -// put it all together +// putting it all together static void undoReturnByRef(FnSymbol* fn) { if (fn->hasFlag(FLAG_VIRTUAL)) return; // skip these for now @@ -868,3 +841,90 @@ static void undoReturnByRef(FnSymbol* fn) { transformRetTempDef(defInfo); } + +///////////////////////////////////////////////////////////////////////////// +// collapseTrivialMoves() converts: +// move(source, expr) // move1 +// move(dest, source) // move2 +// provided: +// 'move1' and 'move2' are adjacent or have only DefExprs in between +// 'source' has no other references +// to: +// move(dest, expr) + +// In some cases codegen adds an appropriate dereference, widening, etc. +// for a symbol-to-symbol move and not for a call-to-symbol move. +// So collapse only moves for which such additions are not needed. +static bool okSymbol(Symbol* sym) { + return sym->qual == QUAL_VAL || sym->qual == QUAL_CONST_VAL; +} + +static bool okToCollapse(SymExpr* dest, SymExpr* source) { + Symbol *destSym = dest->symbol(), *sourceSym = source->symbol(); + return + destSym->type == sourceSym->type && + ! sourceSym->hasEitherFlag(FLAG_CONFIG, FLAG_EXPORT) && + okSymbol(destSym) && okSymbol(sourceSym); +} + +static bool closeEnough(Expr* move1, Expr* move2) { + Expr* curr = move1; + for (int dist = 0; dist < 5; dist++) { // heuristically allow <=5 defs + curr = curr->next; + if (curr == move2) return true; + if (!curr || ! isDefExpr(curr)) return false; + } + return false; +} + +// Returns 'move1', or NULL if the desired pattern is not present. +// 'sourceSE' is the SymExpr for 'source' in 'move2'. +static CallExpr* singleMoveTo(CallExpr* move2, SymExpr* sourceSE) { + Symbol* source = sourceSE->symbol(); + SymExpr* otherSE = source->firstSymExpr(); + // Specialize for exactly two references to 'source'. + if (otherSE == sourceSE) { + // need source -> sourceSE -> otherSE -> NULL + otherSE = otherSE->symbolSymExprsNext; + if (otherSE == nullptr || otherSE->symbolSymExprsNext != nullptr) + return nullptr; // 1 or >2 references + } + else { + // need source -> otherSE -> sourceSE -> NULL + if (otherSE->symbolSymExprsNext != sourceSE || + sourceSE->symbolSymExprsNext != nullptr) + return nullptr; // >2 references + } + + CallExpr* move1 = toCallExpr(otherSE->parentExpr); + if (move1 != nullptr && + otherSE == move1->get(1) && + move1->isPrimitive(PRIM_MOVE) && + (move2 == move1->next || closeEnough(move1, move2)) ) + return move1; + else + return nullptr; +} + +static void collapseTrivialMoves() { +// Empirically, running collapseTrivialMoves() the second time +// would not result in any additional removals. +// This work could be done on a per-function basis using collectCallExprs(). +// Simply traversing 'gCallExprs' avoids the overhead of collectCallExprs(). + for_alive_in_Vec(CallExpr, move2, gCallExprs) { + if (move2->isPrimitive(PRIM_MOVE)) + if (SymExpr* dest = toSymExpr(move2->get(1))) + while (true) { + if (SymExpr* source = toSymExpr(move2->get(2))) + if (okToCollapse(dest, source)) + if (CallExpr* move1 = singleMoveTo(move2, source)) + { + move1->remove(); + source->replace(move1->get(2)->remove()); + source->symbol()->defPoint->remove(); + continue; // will check for another move1 to reduce with move2 + } + break; // no further opportunities with move2 + } // while + } // for +} From f4ab559e32e31ef64991850baff2bc2b0b199f70 Mon Sep 17 00:00:00 2001 From: Vassily Litvinov Date: Fri, 1 Mar 2024 19:32:14 -0800 Subject: [PATCH 3/4] Add --no-return-by-ref Signed-off-by: Vassily Litvinov --- compiler/include/driver.h | 3 ++- compiler/main/driver.cpp | 6 ++++-- compiler/passes/denormalize.cpp | 18 +++++++++++------- doc/rst/usingchapel/debugging.rst | 11 +++++++---- test/compflags/returnByRef/ssca2/COMPOPTS | 1 + test/compflags/returnByRef/ssca2/SSCA2_Modules | 1 + .../returnByRef/ssca2/SSCA2_main.1Dtorus.good | 1 + .../returnByRef/ssca2/SSCA2_main.2Dtorus.good | 1 + .../returnByRef/ssca2/SSCA2_main.3Dtorus.good | 1 + .../returnByRef/ssca2/SSCA2_main.4Dtorus.good | 1 + .../returnByRef/ssca2/SSCA2_main.RMAT.good | 1 + .../returnByRef/ssca2/SSCA2_main.chpl | 1 + .../returnByRef/ssca2/SSCA2_main.compopts | 1 + .../compflags/returnByRef/ssca2/SSCA2_main.dir | 1 + .../returnByRef/ssca2/SSCA2_main.execopts | 1 + .../returnByRef/ssca2/SSCA2_main.numlocales | 1 + .../returnByRef/ssca2/SSCA2_main.prediff | 1 + util/chpl-completion.bash | 3 +++ util/cron/test-perf.chapcs.playground.bash | 10 +++++----- 19 files changed, 45 insertions(+), 19 deletions(-) create mode 100644 test/compflags/returnByRef/ssca2/COMPOPTS create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_Modules create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.1Dtorus.good create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.2Dtorus.good create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.3Dtorus.good create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.4Dtorus.good create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.RMAT.good create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.chpl create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.compopts create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.dir create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.execopts create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.numlocales create mode 120000 test/compflags/returnByRef/ssca2/SSCA2_main.prediff diff --git a/compiler/include/driver.h b/compiler/include/driver.h index 8f0820066270..7c27cbee9ecb 100644 --- a/compiler/include/driver.h +++ b/compiler/include/driver.h @@ -193,6 +193,8 @@ extern bool fNoEarlyDeinit; extern bool fNoCopyElision; extern bool fCompileTimeNilChecking; extern bool fInferImplementsStmts; +extern bool fIteratorContexts; +extern bool fReturnByRef; extern bool fOverrideChecking; extern int ffloatOpt; extern int fMaxCIdentLen; @@ -253,7 +255,6 @@ extern bool fReportOptimizedOn; extern bool fReportPromotion; extern bool fReportScalarReplace; extern bool fReportGpu; -extern bool fIteratorContexts; extern bool fReportContextAdj; extern bool fReportDeadBlocks; extern bool fReportDeadModules; diff --git a/compiler/main/driver.cpp b/compiler/main/driver.cpp index d3e92b1abd25..af4daa2e1dda 100644 --- a/compiler/main/driver.cpp +++ b/compiler/main/driver.cpp @@ -263,6 +263,8 @@ bool fNoEarlyDeinit = false; bool fNoCopyElision = false; bool fCompileTimeNilChecking = true; bool fInferImplementsStmts = false; +bool fIteratorContexts = false; +bool fReturnByRef = true; bool fOverrideChecking = true; bool fieeefloat = false; int ffloatOpt = 0; // 0 -> backend default; -1 -> strict; 1 -> opt @@ -295,7 +297,6 @@ bool fReportOptimizeForallUnordered = false; bool fReportPromotion = false; bool fReportScalarReplace = false; bool fReportGpu = false; -bool fIteratorContexts = false; bool fReportContextAdj = false; bool fReportDeadBlocks = false; bool fReportDeadModules = false; @@ -1411,7 +1412,6 @@ static ArgumentDescription arg_desc[] = { {"report-promotion", ' ', NULL, "Print information about scalar promotion", "F", &fReportPromotion, NULL, NULL}, {"report-scalar-replace", ' ', NULL, "Print scalar replacement stats", "F", &fReportScalarReplace, NULL, NULL}, {"report-gpu", ' ', NULL, "Print information about what loops are and are not GPU eligible", "F", &fReportGpu, NULL, NULL}, - {"iterator-contexts", ' ', NULL, "Handle iterator contexts", "F", &fIteratorContexts, NULL, NULL}, {"report-context-adjustments", ' ', NULL, "Print debugging information while handling iterator contexts", "F", &fReportContextAdj, NULL, NULL}, {"", ' ', NULL, "Developer Flags -- Miscellaneous", NULL, NULL, NULL, NULL}, @@ -1441,6 +1441,7 @@ static ArgumentDescription arg_desc[] = { {"compile-time-nil-checking", ' ', NULL, "Enable [disable] compile-time nil checking", "N", &fCompileTimeNilChecking, "CHPL_COMPILE_TIME_NIL_CHECKS", NULL}, {"infer-implements-decls", ' ', NULL, "Enable [disable] inference of implements-declarations", "N", &fInferImplementsStmts, "CHPL_INFER_IMPLEMENTS_DECLS", NULL}, {"interleave-memory", ' ', NULL, "Enable [disable] memory interleaving", "N", &fEnableMemInterleaving, "CHPL_INTERLEAVE_MEMORY", NULL}, + {"iterator-contexts", ' ', NULL, "Handle iterator contexts", "N", &fIteratorContexts, NULL, NULL}, {"ignore-errors", ' ', NULL, "[Don't] attempt to ignore errors", "N", &ignore_errors, "CHPL_IGNORE_ERRORS", NULL}, {"ignore-user-errors", ' ', NULL, "[Don't] attempt to ignore user errors", "N", &ignore_user_errors, "CHPL_IGNORE_USER_ERRORS", NULL}, {"ignore-errors-for-pass", ' ', NULL, "[Don't] attempt to ignore errors until the end of the pass in which they occur", "N", &ignore_errors_for_pass, "CHPL_IGNORE_ERRORS_FOR_PASS", NULL}, @@ -1477,6 +1478,7 @@ static ArgumentDescription arg_desc[] = { {"remove-empty-records", ' ', NULL, "Enable [disable] empty record removal", "n", &fNoRemoveEmptyRecords, "CHPL_DISABLE_REMOVE_EMPTY_RECORDS", NULL}, {"remove-unreachable-blocks", ' ', NULL, "[Don't] remove unreachable blocks after resolution", "N", &fRemoveUnreachableBlocks, "CHPL_REMOVE_UNREACHABLE_BLOCKS", NULL}, {"replace-array-accesses-with-ref-temps", ' ', NULL, "Enable [disable] replacing array accesses with reference temps (experimental)", "N", &fReplaceArrayAccessesWithRefTemps, NULL, NULL }, + {"return-by-ref", ' ', NULL, "Enable return-by-ref of structs in the generated code", "N", &fReturnByRef, NULL, NULL}, {"incremental", ' ', NULL, "Enable [disable] using incremental compilation", "N", &fIncrementalCompilation, "CHPL_INCREMENTAL_COMP", NULL}, {"minimal-modules", ' ', NULL, "Enable [disable] using minimal modules", "N", &fMinimalModules, "CHPL_MINIMAL_MODULES", NULL}, {"parallel-make", 'j', NULL, "Specify degree of parallelism for C back-end", "I", &fParMake, "CHPL_PAR_MAKE", &turnIncrementalOn}, diff --git a/compiler/passes/denormalize.cpp b/compiler/passes/denormalize.cpp index bea4a610bcd3..6ec36f3ec806 100644 --- a/compiler/passes/denormalize.cpp +++ b/compiler/passes/denormalize.cpp @@ -92,7 +92,8 @@ void denormalize(void) { forv_Vec(FnSymbol, fn, gFnSymbols) { // remove unused epilogue labels removeUnnecessaryGotos(fn, true); - if (fn->hasFlag(FLAG_FN_RETARG)) undoReturnByRef(fn); + if (!fReturnByRef && fn->hasFlag(FLAG_FN_RETARG)) + undoReturnByRef(fn); bool isFirstRound = true; do { @@ -862,8 +863,10 @@ static bool okSymbol(Symbol* sym) { static bool okToCollapse(SymExpr* dest, SymExpr* source) { Symbol *destSym = dest->symbol(), *sourceSym = source->symbol(); return - destSym->type == sourceSym->type && - ! sourceSym->hasEitherFlag(FLAG_CONFIG, FLAG_EXPORT) && + destSym->type == sourceSym->type && + ! sourceSym->hasFlag(FLAG_CONFIG) && + ! sourceSym->hasFlag(FLAG_EXPORT) && + ! sourceSym->hasFlag(FLAG_EXTERN) && okSymbol(destSym) && okSymbol(sourceSym); } @@ -907,10 +910,11 @@ static CallExpr* singleMoveTo(CallExpr* move2, SymExpr* sourceSE) { } static void collapseTrivialMoves() { -// Empirically, running collapseTrivialMoves() the second time -// would not result in any additional removals. -// This work could be done on a per-function basis using collectCallExprs(). -// Simply traversing 'gCallExprs' avoids the overhead of collectCallExprs(). + // Empirically, running collapseTrivialMoves() the second time + // would not result in any additional removals. So, do it just once. + // + // This work could be done on a per-function basis using collectCallExprs(). + // Simply traversing 'gCallExprs' avoids the overhead of collectCallExprs(). for_alive_in_Vec(CallExpr, move2, gCallExprs) { if (move2->isPrimitive(PRIM_MOVE)) if (SymExpr* dest = toSymExpr(move2->get(1))) diff --git a/doc/rst/usingchapel/debugging.rst b/doc/rst/usingchapel/debugging.rst index d7598e80faf8..e8fbc4328360 100644 --- a/doc/rst/usingchapel/debugging.rst +++ b/doc/rst/usingchapel/debugging.rst @@ -80,7 +80,7 @@ executable. This can be done in two steps. .. code-block:: bash - chpl -g --target-compiler=gnu --savec --preserve-inlined-line-numbers --no-munge-user-idents + chpl -g --target-compiler=gnu --savec --preserve-inlined-line-numbers --no-munge-user-idents --no-return-by-ref --no-inline For more details on these settings, read the rest of this section. @@ -101,7 +101,7 @@ Building the Application ~~~~~~~~~~~~~~~~~~~~~~~~ The following flags can be useful for making the generated C more amenable to -debugging. +debugging. Any of them can be omitted as desired. =================================== ========================================= Flag Description @@ -113,13 +113,16 @@ debugging. ``--preserve-inlined-line-numbers`` When code gets inlined (e.g. replacing a function call with the function body) maintain the filename and line number - information of the original function - call. + information of the original function. ``--no-munge-user-idents`` Don't munge user identifiers (e.g. variable or function names). Munging typically prevents conflicts with identifiers in external code but makes debugging harder. + ``--no-return-by-ref`` Don't use an extra reference argument + when compiling a Chapel function that + returns a record. + ``--no-inline`` Avoid inlining in many cases. =================================== ========================================= Notes on munging diff --git a/test/compflags/returnByRef/ssca2/COMPOPTS b/test/compflags/returnByRef/ssca2/COMPOPTS new file mode 100644 index 000000000000..dec19b28e263 --- /dev/null +++ b/test/compflags/returnByRef/ssca2/COMPOPTS @@ -0,0 +1 @@ +--no-return-by-ref diff --git a/test/compflags/returnByRef/ssca2/SSCA2_Modules b/test/compflags/returnByRef/ssca2/SSCA2_Modules new file mode 120000 index 000000000000..c8fc61d988f9 --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_Modules @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_Modules \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.1Dtorus.good b/test/compflags/returnByRef/ssca2/SSCA2_main.1Dtorus.good new file mode 120000 index 000000000000..b34c034c1c7b --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.1Dtorus.good @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.1Dtorus.good \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.2Dtorus.good b/test/compflags/returnByRef/ssca2/SSCA2_main.2Dtorus.good new file mode 120000 index 000000000000..497b5a020c72 --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.2Dtorus.good @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.2Dtorus.good \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.3Dtorus.good b/test/compflags/returnByRef/ssca2/SSCA2_main.3Dtorus.good new file mode 120000 index 000000000000..3af21802ccf3 --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.3Dtorus.good @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.3Dtorus.good \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.4Dtorus.good b/test/compflags/returnByRef/ssca2/SSCA2_main.4Dtorus.good new file mode 120000 index 000000000000..fedb771f31a0 --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.4Dtorus.good @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.4Dtorus.good \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.RMAT.good b/test/compflags/returnByRef/ssca2/SSCA2_main.RMAT.good new file mode 120000 index 000000000000..80b05ea599ed --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.RMAT.good @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.RMAT.good \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.chpl b/test/compflags/returnByRef/ssca2/SSCA2_main.chpl new file mode 120000 index 000000000000..88beb52cf7d7 --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.chpl @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.chpl \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.compopts b/test/compflags/returnByRef/ssca2/SSCA2_main.compopts new file mode 120000 index 000000000000..66089402c75d --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.compopts @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.compopts \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.dir b/test/compflags/returnByRef/ssca2/SSCA2_main.dir new file mode 120000 index 000000000000..77847ef6062d --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.dir @@ -0,0 +1 @@ +../../../studies/ssca2/main \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.execopts b/test/compflags/returnByRef/ssca2/SSCA2_main.execopts new file mode 120000 index 000000000000..7da6515e8dc4 --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.execopts @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.execopts \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.numlocales b/test/compflags/returnByRef/ssca2/SSCA2_main.numlocales new file mode 120000 index 000000000000..9e7a6207885d --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.numlocales @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.numlocales \ No newline at end of file diff --git a/test/compflags/returnByRef/ssca2/SSCA2_main.prediff b/test/compflags/returnByRef/ssca2/SSCA2_main.prediff new file mode 120000 index 000000000000..0b8c62a9f741 --- /dev/null +++ b/test/compflags/returnByRef/ssca2/SSCA2_main.prediff @@ -0,0 +1 @@ +SSCA2_main.dir/SSCA2_main.prediff \ No newline at end of file diff --git a/util/chpl-completion.bash b/util/chpl-completion.bash index 79e8f958ab57..d445d79740b7 100644 --- a/util/chpl-completion.bash +++ b/util/chpl-completion.bash @@ -229,6 +229,7 @@ _chpl () --no-io-deserialize-readThis \ --no-io-gen-serialization \ --no-io-serialize-writeThis \ +--no-iterator-contexts \ --no-library-ml-debug \ --no-lifetime-checking \ --no-live-analysis \ @@ -278,6 +279,7 @@ _chpl () --no-report-auto-local-access \ --no-report-blocking \ --no-resolve-concrete-fns \ +--no-return-by-ref \ --no-scalar-replacement \ --no-specialize \ --no-split-initialization \ @@ -363,6 +365,7 @@ _chpl () --report-scalar-replace \ --report-vectorized-loops \ --resolve-concrete-fns \ +--return-by-ref \ --savec \ --scalar-replace-limit \ --scalar-replacement \ diff --git a/util/cron/test-perf.chapcs.playground.bash b/util/cron/test-perf.chapcs.playground.bash index ea768fc29c72..295a9629d08d 100755 --- a/util/cron/test-perf.chapcs.playground.bash +++ b/util/cron/test-perf.chapcs.playground.bash @@ -25,11 +25,11 @@ export CHPL_NIGHTLY_TEST_CONFIG_NAME="perf.chapcs.playground" # 4) Update START_DATE to be today, using the format mm/dd/yy # -# Test performance without the return-by-reference transformation -GITHUB_USER=vasslitvinov -GITHUB_BRANCH=denorm-return-by-ref -SHORT_NAME=denorm-ret-by-ref -START_DATE=02/08/24 +# Test performance on the main branch -- the playground is currently unused +GITHUB_USER=chapel-lang +GITHUB_BRANCH=main +SHORT_NAME=main +START_DATE=03/02/24 git branch -D $GITHUB_USER-$GITHUB_BRANCH git checkout -b $GITHUB_USER-$GITHUB_BRANCH From 1723d62677ca1cd3afad1f27270f45a88657f0c1 Mon Sep 17 00:00:00 2001 From: Vassily Litvinov Date: Tue, 5 Mar 2024 12:10:51 -0800 Subject: [PATCH 4/4] Follow up on review feedback Signed-off-by: Vassily Litvinov --- compiler/passes/denormalize.cpp | 54 +++++++++++++++------------------ 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/compiler/passes/denormalize.cpp b/compiler/passes/denormalize.cpp index 6ec36f3ec806..7dba8b2217da 100644 --- a/compiler/passes/denormalize.cpp +++ b/compiler/passes/denormalize.cpp @@ -680,7 +680,7 @@ inline bool unsafeExprInBetween(Expr* e1, Expr* e2, Expr* exprToMove, ///////////////////////////////////////////////////////////////////////////// // undoReturnByRef() // -// Ultimately we want to avoid introducing return by ref. +// Ultimately we may want to avoid introducing return by ref. // For now, simply undo it as a denormalization step. /////////// @@ -691,15 +691,14 @@ inline bool unsafeExprInBetween(Expr* e1, Expr* e2, Expr* exprToMove, // with: // return(something) -class DefInfoRR { -public: +struct ReturnByRefDef { FnSymbol* fn; // the function that returns by ref - ArgSymbol* refArg; // the function's formal implementing that + ArgSymbol* refArg; // the function's formal used for indirect return bool skipLnFnArgs; // if true, refArg is fn's 3rd-last formal }; -static bool isGoodRefArg(FnSymbol* fn, DefInfoRR& info, - bool skippedLnFnArgs, Expr* argDef) { +static bool isGoodRefArg(FnSymbol* fn, ReturnByRefDef& info, + bool skippedLnFnArgs, Expr* argDef) { ArgSymbol* refArg = toArgSymbol(toDefExpr(argDef)->sym); if (!refArg->hasFlag(FLAG_RETARG)) return false; // give up; maybe look harder? @@ -710,7 +709,7 @@ static bool isGoodRefArg(FnSymbol* fn, DefInfoRR& info, return true; } -static bool acceptableDef(FnSymbol* fn, DefInfoRR& info) { +static bool acceptableDef(FnSymbol* fn, ReturnByRefDef& info) { // isGoodRefArg() fills 'info' if (! isGoodRefArg(fn, info, false, fn->formals.tail) && ! (fn->numFormals() >= 3 && @@ -733,7 +732,7 @@ static bool acceptableDef(FnSymbol* fn, DefInfoRR& info) { return true; } -static void transformRetTempDef(DefInfoRR& info) { +static void transformRetTempDef(ReturnByRefDef& info) { SymExpr* refUse = nullptr; for_SymbolSymExprs(refSE, info.refArg) { if (CallExpr* call = toCallExpr(refSE->parentExpr)) @@ -749,7 +748,7 @@ static void transformRetTempDef(DefInfoRR& info) { CallExpr* assignCall = toCallExpr(refUse->parentExpr); Expr* refValueExpr = assignCall->get(2)->remove(); INT_ASSERT(isSymExpr(refValueExpr)); // ensure it can be used in 'return' - // Do we need to add FLAG_RVV to refValueExpr->symbol()? + // Todo: do we need to add FLAG_RVV to refValueExpr->symbol()? // At this point temps with FLAG_RVV may occur anywhere due to inlining. assignCall->remove(); @@ -772,15 +771,14 @@ static void transformRetTempDef(DefInfoRR& info) { // // collapseTrivialMoves() will reduce to the original 'call_temp = fn(...)' -class UseInfoRR { -public: // in 'call fn(args, ret_tmp)' above, the SymExprs for: +struct ReturnByRefUse { SymExpr* fnSE; // 'fn' SymExpr* tempSE; // 'ret_tmp' }; // Return true if the desired pattern is present. -static bool acceptableUse(DefInfoRR& defInfo, SymExpr* fnUse, - UseInfoRR& useInfo) { +static bool acceptableUse(ReturnByRefDef& defInfo, SymExpr* fnUse, + ReturnByRefUse& useInfo) { CallExpr* call = toCallExpr(fnUse->parentExpr); if (call == nullptr || call->resolvedFunction() != defInfo.fn) return false; @@ -795,10 +793,9 @@ static bool acceptableUse(DefInfoRR& defInfo, SymExpr* fnUse, return true; } -static void transformRetTempUse(UseInfoRR& info) { +static void transformRetTempUse(ReturnByRefUse& info) { Expr* fnCall = info.fnSE->parentExpr; Symbol* retTemp = info.tempSE->symbol(); - //fyi: INT_ASSERT(! retTemp->type->symbol->hasEitherFlag(FLAG_REF, FLAG_WIDE_REF)); // replace: @@ -809,7 +806,7 @@ static void transformRetTempUse(UseInfoRR& info) { SET_LINENO(fnCall); Expr* anchor = fnCall->prev; - BlockStmt* encl = anchor ? NULL : toBlockStmt(fnCall->parentExpr); + BlockStmt* encl = anchor ? nullptr : toBlockStmt(fnCall->parentExpr); Expr* move = new CallExpr(retTemp->isRef() ? PRIM_ASSIGN : PRIM_MOVE, info.tempSE->remove(), fnCall->remove()); if (anchor) anchor->insertAfter(move); @@ -822,22 +819,22 @@ static void transformRetTempUse(UseInfoRR& info) { static void undoReturnByRef(FnSymbol* fn) { if (fn->hasFlag(FLAG_VIRTUAL)) return; // skip these for now - DefInfoRR defInfo; + ReturnByRefDef defInfo; if (! acceptableDef(fn, defInfo)) return; // Make changes only if we can handle all uses of 'fn'. // While checking, store some findings for later use. - std::vector infos; + std::vector infos; for_SymbolSymExprs(use, fn) { - UseInfoRR useInfo; + ReturnByRefUse useInfo; if (acceptableUse(defInfo, use, useInfo)) infos.push_back(useInfo); else return; } - for (UseInfoRR& info: infos) + for (ReturnByRefUse& info: infos) transformRetTempUse(info); transformRetTempDef(defInfo); @@ -860,7 +857,7 @@ static bool okSymbol(Symbol* sym) { return sym->qual == QUAL_VAL || sym->qual == QUAL_CONST_VAL; } -static bool okToCollapse(SymExpr* dest, SymExpr* source) { +static bool canCollapseMoveBetween(SymExpr* dest, SymExpr* source) { Symbol *destSym = dest->symbol(), *sourceSym = source->symbol(); return destSym->type == sourceSym->type && @@ -920,14 +917,13 @@ static void collapseTrivialMoves() { if (SymExpr* dest = toSymExpr(move2->get(1))) while (true) { if (SymExpr* source = toSymExpr(move2->get(2))) - if (okToCollapse(dest, source)) - if (CallExpr* move1 = singleMoveTo(move2, source)) - { - move1->remove(); - source->replace(move1->get(2)->remove()); - source->symbol()->defPoint->remove(); - continue; // will check for another move1 to reduce with move2 - } + if (canCollapseMoveBetween(dest, source)) + if (CallExpr* move1 = singleMoveTo(move2, source)) { + move1->remove(); + source->replace(move1->get(2)->remove()); + source->symbol()->defPoint->remove(); + continue; // will check for another move1 to reduce with move2 + } break; // no further opportunities with move2 } // while } // for