Skip to content

Commit

Permalink
Translation to LLVM IR: use LogicalResult instead of bool
Browse files Browse the repository at this point in the history
The translation code predates the introduction of LogicalResult and was relying
on the obsolete LLVM convention of returning false on success.  Change it to
use MLIR's LogicalResult abstraction instead. NFC.

PiperOrigin-RevId: 262589432
  • Loading branch information
tensorflower-gardener committed Aug 9, 2019
1 parent c02d99f commit d936bf7
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 38 deletions.
11 changes: 6 additions & 5 deletions third_party/mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class ModuleTranslation {
T translator(m);
translator.llvmModule = std::move(llvmModule);
translator.convertGlobals();
if (translator.convertFunctions())
if (failed(translator.convertFunctions()))
return nullptr;

return std::move(translator.llvmModule);
Expand All @@ -68,15 +68,16 @@ class ModuleTranslation {
explicit ModuleTranslation(ModuleOp module) : mlirModule(module) {}
virtual ~ModuleTranslation() {}

virtual bool convertOperation(Operation &op, llvm::IRBuilder<> &builder);
virtual LogicalResult convertOperation(Operation &op,
llvm::IRBuilder<> &builder);
static std::unique_ptr<llvm::Module> prepareLLVMModule(ModuleOp m);

private:
bool convertFunctions();
LogicalResult convertFunctions();
void convertGlobals();
bool convertOneFunction(FuncOp func);
LogicalResult convertOneFunction(FuncOp func);
void connectPHINodes(FuncOp func);
bool convertBlock(Block &bb, bool ignoreArguments);
LogicalResult convertBlock(Block &bb, bool ignoreArguments);

template <typename Range>
SmallVector<llvm::Value *, 8> lookupValues(Range &&values);
Expand Down
4 changes: 2 additions & 2 deletions third_party/mlir/lib/Target/LLVMIR/ConvertToNVVMIR.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ class ModuleTranslation : public LLVM::ModuleTranslation {
~ModuleTranslation() override {}

protected:
bool convertOperation(Operation &opInst,
llvm::IRBuilder<> &builder) override {
LogicalResult convertOperation(Operation &opInst,
llvm::IRBuilder<> &builder) override {

#include "mlir/LLVMIR/NVVMConversions.inc"

Expand Down
56 changes: 26 additions & 30 deletions third_party/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ SmallVector<llvm::Value *, 8> ModuleTranslation::lookupValues(Range &&values) {
// using the `builder`. LLVM IR Builder does not have a generic interface so
// this has to be a long chain of `if`s calling different functions with a
// different number of arguments.
bool ModuleTranslation::convertOperation(Operation &opInst,
llvm::IRBuilder<> &builder) {
LogicalResult ModuleTranslation::convertOperation(Operation &opInst,
llvm::IRBuilder<> &builder) {
auto extractPosition = [](ArrayAttr attr) {
SmallVector<unsigned, 4> position;
position.reserve(attr.size());
Expand Down Expand Up @@ -228,33 +228,33 @@ bool ModuleTranslation::convertOperation(Operation &opInst,
llvm::Value *result = convertCall(opInst);
if (opInst.getNumResults() != 0) {
valueMapping[opInst.getResult(0)] = result;
return false;
return success();
}
// Check that LLVM call returns void for 0-result functions.
return !result->getType()->isVoidTy();
return success(result->getType()->isVoidTy());
}

// Emit branches. We need to look up the remapped blocks and ignore the block
// arguments that were transformed into PHI nodes.
if (auto brOp = dyn_cast<LLVM::BrOp>(opInst)) {
builder.CreateBr(blockMapping[brOp.getSuccessor(0)]);
return false;
return success();
}
if (auto condbrOp = dyn_cast<LLVM::CondBrOp>(opInst)) {
builder.CreateCondBr(valueMapping.lookup(condbrOp.getOperand(0)),
blockMapping[condbrOp.getSuccessor(0)],
blockMapping[condbrOp.getSuccessor(1)]);
return false;
return success();
}

opInst.emitError("unsupported or non-LLVM operation: ") << opInst.getName();
return true;
return opInst.emitError("unsupported or non-LLVM operation: ")
<< opInst.getName();
}

// Convert block to LLVM IR. Unless `ignoreArguments` is set, emit PHI nodes
// to define values corresponding to the MLIR block arguments. These nodes
// are not connected to the source basic blocks, which may not exist yet.
bool ModuleTranslation::convertBlock(Block &bb, bool ignoreArguments) {
LogicalResult ModuleTranslation::convertBlock(Block &bb, bool ignoreArguments) {
llvm::IRBuilder<> builder(blockMapping[&bb]);

// Before traversing operations, make block arguments available through
Expand All @@ -269,11 +269,9 @@ bool ModuleTranslation::convertBlock(Block &bb, bool ignoreArguments) {
std::distance(predecessors.begin(), predecessors.end());
for (auto *arg : bb.getArguments()) {
auto wrappedType = arg->getType().dyn_cast<LLVM::LLVMType>();
if (!wrappedType) {
emitError(bb.front().getLoc(),
"block argument does not have an LLVM type");
return true;
}
if (!wrappedType)
return emitError(bb.front().getLoc(),
"block argument does not have an LLVM type");
llvm::Type *type = wrappedType.getUnderlyingType();
llvm::PHINode *phi = builder.CreatePHI(type, numPredecessors);
valueMapping[arg] = phi;
Expand All @@ -282,11 +280,11 @@ bool ModuleTranslation::convertBlock(Block &bb, bool ignoreArguments) {

// Traverse operations.
for (auto &op : bb) {
if (convertOperation(op, builder))
return true;
if (failed(convertOperation(op, builder)))
return failure();
}

return false;
return success();
}

// Create named global variables that correspond to llvm.global definitions.
Expand Down Expand Up @@ -379,7 +377,7 @@ static llvm::SetVector<Block *> topologicalSort(FuncOp f) {
return blocks;
}

bool ModuleTranslation::convertOneFunction(FuncOp func) {
LogicalResult ModuleTranslation::convertOneFunction(FuncOp func) {
// Clear the block and value mappings, they are only relevant within one
// function.
blockMapping.clear();
Expand All @@ -396,11 +394,9 @@ bool ModuleTranslation::convertOneFunction(FuncOp func) {
// NB: Attribute already verified to be boolean, so check if we can indeed
// attach the attribute to this argument, based on its type.
auto argTy = mlirArg->getType().dyn_cast<LLVM::LLVMType>();
if (!argTy.getUnderlyingType()->isPointerTy()) {
func.emitError(
if (!argTy.getUnderlyingType()->isPointerTy())
return func.emitError(
"llvm.noalias attribute attached to LLVM non-pointer argument");
return true;
}
if (attr.getValue())
llvmArg.addAttr(llvm::Attribute::AttrKind::NoAlias);
}
Expand All @@ -421,17 +417,17 @@ bool ModuleTranslation::convertOneFunction(FuncOp func) {
auto blocks = topologicalSort(func);
for (auto indexedBB : llvm::enumerate(blocks)) {
auto *bb = indexedBB.value();
if (convertBlock(*bb, /*ignoreArguments=*/indexedBB.index() == 0))
return true;
if (failed(convertBlock(*bb, /*ignoreArguments=*/indexedBB.index() == 0)))
return failure();
}

// Finally, after all blocks have been traversed and values mapped, connect
// the PHI nodes to the results of preceding blocks.
connectPHINodes(func);
return false;
return success();
}

bool ModuleTranslation::convertFunctions() {
LogicalResult ModuleTranslation::convertFunctions() {
// Declare all functions first because there may be function calls that form a
// call graph with cycles.
for (FuncOp function : mlirModule.getOps<FuncOp>()) {
Expand All @@ -442,7 +438,7 @@ bool ModuleTranslation::convertFunctions() {
convertFunctionType(llvmModule->getContext(), function.getType(),
function.getLoc(), isVarArgs);
if (!functionType)
return true;
return failure();
llvm::FunctionCallee llvmFuncCst =
llvmModule->getOrInsertFunction(function.getName(), functionType);
assert(isa<llvm::Function>(llvmFuncCst.getCallee()));
Expand All @@ -456,11 +452,11 @@ bool ModuleTranslation::convertFunctions() {
if (function.isExternal())
continue;

if (convertOneFunction(function))
return true;
if (failed(convertOneFunction(function)))
return failure();
}

return false;
return success();
}

std::unique_ptr<llvm::Module> ModuleTranslation::prepareLLVMModule(ModuleOp m) {
Expand Down
2 changes: 1 addition & 1 deletion third_party/mlir/tools/mlir-tblgen/LLVMIRConversionGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ static bool emitOneBuilder(const Record &record, raw_ostream &os) {
os << "if (auto op = dyn_cast<" << op.getQualCppClassName()
<< ">(opInst)) {\n";
os << bs.str() << builderStrRef << "\n";
os << " return false;\n";
os << " return success();\n";
os << "}\n";

return true;
Expand Down

0 comments on commit d936bf7

Please sign in to comment.