[flang][OpenMP] Revert array element reduction lowering - #215622
Open
tblah wants to merge 2 commits into
Open
Conversation
Revert llvm#196094 and its follow-up llvm#209701. The expression override mechanism does not preserve reduction-object identity across all data environments, leading to incorrect lowering for procedure-local and nested private arrays. Restore the pre-llvm#196094 lowering. Keep coverage showing that array-element constructs compile through the boxed-array reduction path. This was in response to this comment: llvm#196094 (comment) Fixing exactly the bug in the comment wasn't hard but AI code review found a large number of follow on bugs so I think the design needs a rethink, and definitely shouldn't be included in the LLVM release. Assisted-by: Codex
Contributor
Author
|
I made this as a PR instead of using the bot because the patch wouldn't rebase cleanly |
llvmorg-github-actions
Bot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-openmp Author: Tom Eccles (tblah) ChangesRevert #196094 and its follow-up #209701. The expression override mechanism does not preserve reduction-object identity across all data environments, leading to incorrect lowering for procedure-local and nested private arrays. Restore the pre-#196094 lowering. Keep coverage showing that array-element constructs compile through the boxed-array reduction path. This was in response to this comment: Fixing exactly the bug in the comment wasn't hard but AI code review found a large number of follow on bugs so I think the design needs a rethink, and definitely shouldn't be included in the LLVM release. See main-branch PR: #215617 Assisted-by: Codex Patch is 52.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/215622.diff 10 Files Affected:
diff --git a/flang/include/flang/Lower/Support/ReductionProcessor.h b/flang/include/flang/Lower/Support/ReductionProcessor.h
index 7173fa9e33a31..a949da875b3a2 100644
--- a/flang/include/flang/Lower/Support/ReductionProcessor.h
+++ b/flang/include/flang/Lower/Support/ReductionProcessor.h
@@ -13,7 +13,6 @@
#ifndef FORTRAN_LOWER_REDUCTIONPROCESSOR_H
#define FORTRAN_LOWER_REDUCTIONPROCESSOR_H
-#include "flang/Lower/AbstractConverter.h"
#include "flang/Lower/OpenMP/Clauses.h"
#include "flang/Optimizer/Builder/FIRBuilder.h"
#include "flang/Optimizer/Dialect/FIRType.h"
@@ -22,7 +21,6 @@
#include "flang/Semantics/type.h"
#include "mlir/IR/Location.h"
#include "mlir/IR/Types.h"
-#include "llvm/ADT/ArrayRef.h"
namespace mlir {
namespace omp {
@@ -170,16 +168,8 @@ class ReductionProcessor {
llvm::SmallVectorImpl<bool> &reduceVarByRef,
llvm::SmallVectorImpl<mlir::Attribute> &reductionDeclSymbols,
const llvm::SmallVectorImpl<const semantics::Symbol *> &reductionSymbols,
- llvm::ArrayRef<Object> reductionObjects, lower::SymMap &symMap,
llvm::DenseMap<const semantics::Symbol *, mlir::Value>
*reductionVarCache = nullptr);
-
- /// Check if an expression is lowered as a Reduction object. This ensures
- /// reductions such as Array Elements are properly represented, rather than
- /// reducing the full array.
- // TODO support more types of objects
- // to avoid Reduction clauses being represented in FIR as full arrays.
- static bool isExpressionLoweredAsReductionObject(const Object *object);
};
template <typename FloatOp, typename IntegerOp>
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index e2c895351c977..f527e807f935c 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -2416,8 +2416,7 @@ class FirConverter : public Fortran::lower::AbstractConverter {
Fortran::lower::omp::ReductionProcessor rp;
bool result = rp.processReductionArguments<fir::DeclareReductionOp>(
toLocation(), *this, info.reduceOperatorList, reduceVars,
- reduceVarByRef, reductionDeclSymbols, info.reduceSymList,
- /*reductionObjects=*/{}, getSymbolMap());
+ reduceVarByRef, reductionDeclSymbols, info.reduceSymList);
if (!result)
TODO(toLocation(), "Lowering unrecognised reduction type");
diff --git a/flang/lib/Lower/ConvertExprToHLFIR.cpp b/flang/lib/Lower/ConvertExprToHLFIR.cpp
index 6f718a8eb5926..bac7b6cbd2a89 100644
--- a/flang/lib/Lower/ConvertExprToHLFIR.cpp
+++ b/flang/lib/Lower/ConvertExprToHLFIR.cpp
@@ -975,20 +975,11 @@ class HlfirDesignatorBuilder {
mlir::Location loc;
};
-static mlir::Value
-findOverriddenExprValue(const Fortran::lower::ExprToValueMap &map,
- const Fortran::lower::SomeExpr &expr);
-
hlfir::EntityWithAttributes HlfirDesignatorBuilder::genDesignatorExpr(
const Fortran::lower::SomeExpr &designatorExpr,
bool vectorSubscriptDesignatorToValue) {
// Expr<SomeType> plumbing to unwrap Designator<T> and call
// gen(Designator<T>.u).
- if (const Fortran::lower::ExprToValueMap *map =
- getConverter().getExprOverrides()) {
- if (mlir::Value value = findOverriddenExprValue(*map, designatorExpr))
- return hlfir::EntityWithAttributes{value};
- }
return Fortran::common::visit(
[&](const auto &x) -> hlfir::EntityWithAttributes {
using T = std::decay_t<decltype(x)>;
@@ -1561,30 +1552,6 @@ static bool hasDeferredCharacterLength(const Fortran::semantics::Symbol &sym) {
type->characterTypeSpec().length().isDeferred();
}
-static mlir::Value
-findOverriddenExprValue(const Fortran::lower::ExprToValueMap &map,
- const Fortran::lower::SomeExpr &expr) {
- if (auto match = map.find(&expr); match != map.end())
- return match->second;
-
- // The map uses pointer identity, but the some expressions
- // (e.g. a(2)) may appear at multiple AST nodes with different addresses.
- // Fall back to structural comparison via ArrayRef::operator==.
- for (auto [key, value] : map) {
- if (Fortran::lower::isEqual(key, &expr))
- return value;
- auto keyRef = Fortran::evaluate::ExtractDataRef(*key);
- auto exprRef = Fortran::evaluate::ExtractDataRef(expr);
- if (keyRef && exprRef) {
- auto *keyArray = std::get_if<Fortran::evaluate::ArrayRef>(&keyRef->u);
- auto *exprArray = std::get_if<Fortran::evaluate::ArrayRef>(&exprRef->u);
- if (keyArray && exprArray && *keyArray == *exprArray)
- return value;
- }
- }
- return {};
-}
-
/// Lower Expr to HLFIR.
class HlfirBuilder {
public:
@@ -1598,12 +1565,12 @@ class HlfirBuilder {
if (const Fortran::lower::ExprToValueMap *map =
getConverter().getExprOverrides()) {
if constexpr (std::is_same_v<T, Fortran::evaluate::SomeType>) {
- if (mlir::Value value = findOverriddenExprValue(*map, expr))
- return hlfir::EntityWithAttributes{value};
+ if (auto match = map->find(&expr); match != map->end())
+ return hlfir::EntityWithAttributes{match->second};
} else {
Fortran::lower::SomeExpr someExpr = toEvExpr(expr);
- if (mlir::Value value = findOverriddenExprValue(*map, someExpr))
- return hlfir::EntityWithAttributes{value};
+ if (auto match = map->find(&someExpr); match != map->end())
+ return hlfir::EntityWithAttributes{match->second};
}
}
return Fortran::common::visit([&](const auto &x) { return gen(x); },
@@ -1645,12 +1612,6 @@ class HlfirBuilder {
template <typename T>
hlfir::EntityWithAttributes
gen(const Fortran::evaluate::Designator<T> &designator) {
- if (const Fortran::lower::ExprToValueMap *map =
- getConverter().getExprOverrides()) {
- Fortran::lower::SomeExpr someExpr = toEvExpr(designator);
- if (mlir::Value value = findOverriddenExprValue(*map, someExpr))
- return hlfir::EntityWithAttributes{value};
- }
return HlfirDesignatorBuilder(getLoc(), getConverter(), getSymMap(),
getStmtCtx())
.gen(designator.u);
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 26438ef968008..cfa3673a56f08 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -1680,7 +1680,7 @@ bool ClauseProcessor::processInReduction(
currentLocation, converter,
std::get<typename omp::clause::ReductionOperatorList>(clause.t),
inReductionVars, inReduceVarByRef, inReductionDeclSymbols,
- inReductionSyms, inReductionObjects, converter.getSymbolMap()))
+ inReductionSyms))
TODO(currentLocation, "Lowering unrecognised reduction type");
// Copy local lists into the output.
@@ -2098,8 +2098,7 @@ bool ClauseProcessor::processReduction(
currentLocation, converter,
std::get<typename omp::clause::ReductionOperatorList>(clause.t),
reductionVars, reduceVarByRef, reductionDeclSymbols,
- reductionSyms, reductionObjects, converter.getSymbolMap(),
- reductionVarCache))
+ reductionSyms, reductionVarCache))
TODO(currentLocation, "Lowering unrecognised reduction type");
// Copy local lists into the output.
llvm::copy(reductionVars, std::back_inserter(result.reductionVars));
@@ -2128,8 +2127,7 @@ bool ClauseProcessor::processTaskReduction(
currentLocation, converter,
std::get<typename omp::clause::ReductionOperatorList>(clause.t),
taskReductionVars, taskReduceVarByRef, taskReductionDeclSymbols,
- taskReductionSyms, taskReductionObjects,
- converter.getSymbolMap()))
+ taskReductionSyms))
TODO(currentLocation, "Lowering unrecognised reduction type");
// Copy local lists into the output.
llvm::copy(taskReductionVars,
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 401c039f43973..5f7d2b16d74d4 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -66,30 +66,27 @@ DataSharingProcessor::DataSharingProcessor(
lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
const List<Clause> &clauses, lower::pft::Evaluation &eval,
bool shouldCollectPreDeterminedSymbols, bool useDelayedPrivatization,
- lower::SymMap &symTable, bool isTargetPrivatization,
- llvm::ArrayRef<const semantics::Symbol *> symbolsCoveredByReductionElements)
+ lower::SymMap &symTable, bool isTargetPrivatization)
: converter(converter), semaCtx(semaCtx),
firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
shouldCollectPreDeterminedSymbols(shouldCollectPreDeterminedSymbols),
useDelayedPrivatization(useDelayedPrivatization), symTable(symTable),
isTargetPrivatization(isTargetPrivatization), visitor(semaCtx) {
- this->symbolsCoveredByReductionElements.insert(
- symbolsCoveredByReductionElements.begin(),
- symbolsCoveredByReductionElements.end());
eval.visit([&](const auto &functionParserNode) {
parser::Walk(functionParserNode, visitor);
});
}
-DataSharingProcessor::DataSharingProcessor(
- lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
- lower::pft::Evaluation &eval, bool useDelayedPrivatization,
- lower::SymMap &symTable, bool isTargetPrivatization,
- llvm::ArrayRef<const semantics::Symbol *> symbolsCoveredByReductionElements)
- : DataSharingProcessor(
- converter, semaCtx, {}, eval,
- /*shouldCollectPreDeterminedSymols=*/false, useDelayedPrivatization,
- symTable, isTargetPrivatization, symbolsCoveredByReductionElements) {}
+DataSharingProcessor::DataSharingProcessor(lower::AbstractConverter &converter,
+ semantics::SemanticsContext &semaCtx,
+ lower::pft::Evaluation &eval,
+ bool useDelayedPrivatization,
+ lower::SymMap &symTable,
+ bool isTargetPrivatization)
+ : DataSharingProcessor(converter, semaCtx, {}, eval,
+ /*shouldCollectPreDeterminedSymols=*/false,
+ useDelayedPrivatization, symTable,
+ isTargetPrivatization) {}
void DataSharingProcessor::processStep1(
mlir::omp::PrivateClauseOps *clauseOps,
@@ -289,20 +286,6 @@ void DataSharingProcessor::collectSymbolsForPrivatization() {
allPrivatizedSymbols.insert(sym);
}
-bool DataSharingProcessor::isCoveredByReductionElement(
- const semantics::Symbol *sym) const {
- if (symbolsCoveredByReductionElements.contains(sym) ||
- symbolsCoveredByReductionElements.contains(&sym->GetUltimate()))
- return true;
-
- if (const auto *hostAssoc = sym->detailsIf<semantics::HostAssocDetails>())
- return symbolsCoveredByReductionElements.contains(&hostAssoc->symbol()) ||
- symbolsCoveredByReductionElements.contains(
- &hostAssoc->symbol().GetUltimate());
-
- return false;
-}
-
bool DataSharingProcessor::needBarrier() {
// Emit implicit barrier to synchronize threads and avoid data races on
// initialization of firstprivate variables and post-update of lastprivate
@@ -519,11 +502,6 @@ void DataSharingProcessor::collectPrivatizedSymbols(
return false;
if (collectImplicit) {
- // If all uses of a privatisaed variable are covered by an expr in a
- // reduction clause, these should be ignored.
- if (isCoveredByReductionElement(sym))
- return false;
-
// If we're a combined construct with a target region, implicit
// firstprivate captures, should only belong to the target region
// and not be added/captured by later directives. Parallel regions
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index 01a25e041ef15..a9b57cada9d92 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -19,8 +19,6 @@
#include "flang/Parser/parse-tree.h"
#include "flang/Semantics/symbol.h"
#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
-#include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/SmallPtrSet.h"
#include <variant>
namespace mlir {
@@ -109,8 +107,6 @@ class DataSharingProcessor {
bool useDelayedPrivatization;
bool forceHeapAllocationForPrivateDynamicArrays = false;
llvm::SmallPtrSet<const semantics::Symbol *, 16> mightHaveReadHostSym;
- llvm::SmallPtrSet<const semantics::Symbol *, 4>
- symbolsCoveredByReductionElements;
lower::SymMap &symTable;
bool isTargetPrivatization;
OMPConstructSymbolVisitor visitor;
@@ -131,7 +127,6 @@ class DataSharingProcessor {
const omp::ObjectList &objects,
llvm::SetVector<const semantics::Symbol *> &symbolSet);
void collectSymbolsForPrivatization();
- bool isCoveredByReductionElement(const semantics::Symbol *sym) const;
void insertBarrier(mlir::omp::PrivateClauseOps *clauseOps);
void collectDefaultSymbols();
void collectImplicitSymbols();
@@ -160,17 +155,13 @@ class DataSharingProcessor {
lower::pft::Evaluation &eval,
bool shouldCollectPreDeterminedSymbols,
bool useDelayedPrivatization, lower::SymMap &symTable,
- bool isTargetPrivatization = false,
- llvm::ArrayRef<const semantics::Symbol *>
- symbolsCoveredByReductionElements = {});
+ bool isTargetPrivatization = false);
DataSharingProcessor(lower::AbstractConverter &converter,
semantics::SemanticsContext &semaCtx,
lower::pft::Evaluation &eval,
bool useDelayedPrivatization, lower::SymMap &symTable,
- bool isTargetPrivatization = false,
- llvm::ArrayRef<const semantics::Symbol *>
- symbolsCoveredByReductionElements = {});
+ bool isTargetPrivatization = false);
// Privatisation is split into two steps.
// Step1 performs cloning of all privatisation clauses and copying for
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index ed3e989daab7e..eb1c0d7be5f67 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -834,31 +834,18 @@ static void bindEntryBlockArgs(lower::AbstractConverter &converter,
llvm::ArrayRef<mlir::Value> vars,
llvm::ArrayRef<mlir::BlockArgument> args) {
llvm::SmallVector<const semantics::Symbol *> processedSyms;
- llvm::SmallVector<const Object *> processedObjects;
for (const Object &object : objects) {
const semantics::Symbol *sym = object.sym();
if (const auto *commonDet =
sym->detailsIf<semantics::CommonBlockDetails>()) {
- for (auto &mem : commonDet->objects()) {
- processedSyms.push_back(&*mem);
- processedObjects.push_back(&object);
- }
+ llvm::transform(commonDet->objects(), std::back_inserter(processedSyms),
+ [&](const auto &mem) { return &*mem; });
} else {
processedSyms.push_back(sym);
- processedObjects.push_back(&object);
}
}
- assert(processedSyms.size() == processedObjects.size());
- for (auto [sym, var, arg, object] :
- llvm::zip_equal(processedSyms, vars, args, processedObjects)) {
- bool skipBind =
- ReductionProcessor::isExpressionLoweredAsReductionObject(object) ||
- (object && sym->Rank() > 0 &&
- !fir::unwrapUntilSeqType(arg.getType()));
- if (skipBind)
- continue;
-
+ for (auto [sym, var, arg] : llvm::zip_equal(processedSyms, vars, args))
converter.bindSymbol(
*sym,
hlfir::translateToExtendedValue(
@@ -866,7 +853,6 @@ static void bindEntryBlockArgs(lower::AbstractConverter &converter,
/*contiguousHint=*/
evaluate::IsSimplyContiguous(*sym, converter.getFoldingContext()))
.first);
- }
};
// Process in clause name alphabetical order to match block arguments order.
@@ -1343,44 +1329,13 @@ genLoopVars(mlir::Operation *op, lower::AbstractConverter &converter,
// next one would result in 'hlfir.declare' operations being introduced inside
// of a wrapper, which is illegal.
mlir::IRMapping mapper;
- llvm::SmallVector<std::pair<Object, mlir::Value>> mappedReductionObjects;
- auto mapEquivalentReductionObjects =
- [&](const ObjectEntryBlockArgsEntry &entry) {
- for (auto [object, var] : llvm::zip(entry.objects, entry.vars)) {
- for (auto [mappedObject, mappedValue] :
- llvm::reverse(mappedReductionObjects)) {
- if (object.id() == mappedObject.id()) {
- mapper.map(var, mappedValue);
- break;
- }
- }
- }
- };
- auto rememberReductionObjects =
- [&](const ObjectEntryBlockArgsEntry &entry,
- llvm::ArrayRef<mlir::BlockArgument> args) {
- for (auto [object, arg] : llvm::zip(entry.objects, args))
- mappedReductionObjects.emplace_back(object, arg);
- };
-
for (auto [argGeneratingOp, blockArgs] : wrapperArgs) {
- mapEquivalentReductionObjects(blockArgs.inReduction);
- mapEquivalentReductionObjects(blockArgs.reduction);
- mapEquivalentReductionObjects(blockArgs.taskReduction);
-
for (mlir::OpOperand &operand : argGeneratingOp->getOpOperands())
operand.set(mapper.lookupOrDefault(operand.get()));
for (const auto [arg, var] : llvm::zip_equal(
argGeneratingOp->getRegion(0).getArguments(), blockArgs.getVars()))
mapper.map(var, arg);
-
- rememberReductionObjects(blockArgs.inReduction,
- argGeneratingOp.getInReductionBlockArgs());
- rememberReductionObjects(blockArgs.reduction,
- argGeneratingOp.getReductionBlockArgs());
- rememberReductionObjects(blockArgs.taskReduction,
- argGeneratingOp.getTaskReductionBlockArgs());
}
// Bind the entry block arguments of parent wrappers to the corresponding
@@ -1653,186 +1608,6 @@ struct OpWithBodyGenInfo {
bool privatize = true;
};
-static mlir::Value getReductionOverrideValue(fir::FirOpBuilder &builder,
- mlir::Location loc,
- const Object *object,
- mlir::BlockArgument arg) {
- if (hlfir::isFortranEntityWithAttributes(arg))
- return arg;
-
- fir::FortranVariableFlagsAttr attributes;
- llvm::SmallVector<mlir::Value> typeParams;
- auto declareOp = hlfir::DeclareOp::create(
- builder, loc, arg, "omp.reduction.element", nullptr, typeParams, nullptr,
- nullptr, 0, attributes);
- return declareOp.getBase();
-}
-
-static void
-addReductionObjectOverrides(fir::FirOpBuilder &builder, mlir::Location loc,
- lower::ExprToValueMap &overrides,
- const ObjectEntryBlockArgsEntry &entry,
- llvm::ArrayRef<mlir::BlockArgument> blockArgs) {
- if (entry.objects.empty())
- return;
-
- for (auto pair : llvm::zip_equal(entry.objects, blockArgs)) {
- const Object &object = std::get<0>(pair);
- const mlir::BlockArgument &arg = std::get<1>(pair);
- if (!ReductionProcessor::isExpressionLoweredAsReductionObject(&object))
- continue;
- const SomeExpr *expr = &object.ref().value();
-
- // Evict any outer-scope entry for the same array element so the
- // innermost scope always wins regardless of DenseMap iteration order.
- llvm::SmallVector<const SomeExpr *> toEvict;
- for (auto [key, value] : overrides) {
- if (Fortran::lower::isEqual(key, expr)) {
- toEvict.push_back(key);
- }
- }
- ...
[truncated]
|
github-project-automation
Bot
moved this from Needs Triage
to Needs Merge
in LLVM Release Status
Member
|
At commit |
Task and taskloop array-element reductions can introduce both a reduction block argument and an implicit firstprivate block argument for the base array. Sequential symbol binding can then select the wrong argument for references in the construct body. Reject these cases until lowering can distinguish the reduction element from other uses of the base array. Keep supported array-element reduction coverage in the existing test and move the unsupported task forms to focused TODO tests. Assisted-by: Codex
Contributor
Author
|
@tru this is ready to merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revert #196094 and its follow-up #209701. The expression override mechanism does not preserve reduction-object identity across all data environments, leading to incorrect lowering for procedure-local and nested private arrays.
Restore the pre-#196094 lowering. Keep coverage showing that array-element constructs compile through the boxed-array reduction path.
This was in response to this comment:
#196094 (comment)
Fixing exactly the bug in the comment wasn't hard but AI code review found a large number of follow on bugs so I think the design needs a rethink, and definitely shouldn't be included in the LLVM release.
See main-branch PR: #215617
Assisted-by: Codex