[mlir][emitc] Fix indent in CondBranchOp and block label

[xndcn]

Summary

1. Add missing indent in CondBranchOp
2. Remove indent in block label (not sure if it is proper, but looks better)

before:

int64_t simple(int64_t v1, bool v2) {
  int64_t v3;
  int64_t v4;
  int64_t v5;
  int64_t v6;
  int64_t v7;
  label1:
  if (v2) {
  goto label2;
  } else {
  goto label3;
  }
  label2:
  v5 = v1;
  goto label4;

after:

int64_t simple(int64_t v1, bool v2) {
  int64_t v3;
  int64_t v4;
  int64_t v5;
  int64_t v6;
  int64_t v7;
label1:
  if (v2) {
    goto label2;
  } else {
    goto label3;
  }
label2:
  v5 = v1;
  goto label4;

[mehdi_amini]

Can you include the code samples directly in the commit description instead of screenshots? (it won't survive in git otherwise)

[mehdi_amini]

mlir/lib/Target/Cpp/TranslateToCpp.cpp

879

Why is the getOStream() needed here now compared to before?

[xndcn]

mlir/lib/Target/Cpp/TranslateToCpp.cpp

879

Using getOStream() to ignore indent for block label

[marbre]

LGTM. Thanks for improving the EmitC!

If you want me to land this patch for you, please let me know which name and mail I should set for author. Seems you send this patch via the webinterface and not via arc so the author details were lost.

[jpienaar]

mlir/lib/Target/Cpp/TranslateToCpp.cpp

879

This is a little bit fragile: by using the underlying stream it means that the column used is no longer tracked and so this would only work if the start and end state wrt columns are exactly the same (e.g., if the indentation was added before then no indentation would have been added here anymore on the next line and so you'd have the block label indented but next line not, so this relies on this function be called post a newline added but before indentation is added). Please document that here. This seems like something which needs a feature in the indented ostream.

[xndcn]

Thanks! Maybe I can add something like a "UnindentScope"? So we can temporarily print something without any indents.

struct UnindentScope {
    explicit UnindentScope(raw_indented_ostream &os) : os(os) {
      savedIndent = os.currentIndent;
      os.currentIndent = 0;
    }

    ~UnindentScope() {
      os.currentIndent = savedIndent;
    }

    raw_indented_ostream &os;

  private:
    int savedIndent = 0;
  };

[xndcn]

Add a FXIME comment in emitLabel function:

+ FIXME: Add feature in raw_indented_ostream to ignore indent for block
+ label instead of using getOStream.
+ os.getOStream() << getOrCreateName(block) << ":\n";

mlir/lib/Target/Cpp/TranslateToCpp.cpp

static LogicalResult printOperation(CppEmitter &emitter, BranchOp branchOp) {
   if (!(emitter.hasBlockLabel(successor)))
     return branchOp.emitOpError("unable to find label for successor block");
   os << emitter.getOrCreateName(successor);
   return success();
 }
 
 static LogicalResult printOperation(CppEmitter &emitter,
                                     CondBranchOp condBranchOp) {
-  raw_ostream &os = emitter.ostream();
+  raw_indented_ostream &os = emitter.ostream();
   Block &trueSuccessor = *condBranchOp.getTrueDest();
   Block &falseSuccessor = *condBranchOp.getFalseDest();
 
   os << "if (" << emitter.getOrCreateName(condBranchOp.getCondition())
      << ") {\n";
 
+  os.indent();
+
   // If condition is true.
   for (auto pair : llvm::zip(condBranchOp.getTrueOperands(),
                              trueSuccessor.getArguments())) {
     Value &operand = std::get<0>(pair);
     BlockArgument &argument = std::get<1>(pair);
     os << emitter.getOrCreateName(argument) << " = "
        << emitter.getOrCreateName(operand) << ";\n";
   }
 
   os << "goto ";
   if (!(emitter.hasBlockLabel(trueSuccessor))) {
     return condBranchOp.emitOpError("unable to find label for successor block");
   }
   os << emitter.getOrCreateName(trueSuccessor) << ";\n";
-  os << "} else {\n";
+  os.unindent() << "} else {\n";
+  os.indent();
   // If condition is false.
   for (auto pair : llvm::zip(condBranchOp.getFalseOperands(),
                              falseSuccessor.getArguments())) {
     Value &operand = std::get<0>(pair);
     BlockArgument &argument = std::get<1>(pair);
     os << emitter.getOrCreateName(argument) << " = "
        << emitter.getOrCreateName(operand) << ";\n";
   }
 
   os << "goto ";
   if (!(emitter.hasBlockLabel(falseSuccessor))) {
     return condBranchOp.emitOpError()
            << "unable to find label for successor block";
   }
   os << emitter.getOrCreateName(falseSuccessor) << ";\n";
-  os << "}";
+  os.unindent() << "}";
   return success();
 }
 
 static LogicalResult printOperation(CppEmitter &emitter, mlir::CallOp callOp) {
   if (failed(emitter.emitAssignPrefix(*callOp.getOperation())))
     return failure();
 
   raw_ostream &os = emitter.ostream();
default:
     os << "std::tie(";
     interleaveComma(op.getResults(), os,
                     [&](Value result) { os << getOrCreateName(result); });
     os << ") = ";
   }
   return success();
 }
 
 LogicalResult CppEmitter::emitLabel(Block &block) {
   if (!hasBlockLabel(block))
     return block.getParentOp()->emitError("label for block not found");
-  os << getOrCreateName(block) << ":\n";
+  // FIXME: Add feature in `raw_indented_ostream` to ignore indent for block
+  // label instead of using `getOStream`.
+  os.getOStream() << getOrCreateName(block) << ":\n";
   return success();
 }
 
 LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) {
   LogicalResult status =
       llvm::TypeSwitch<Operation *, LogicalResult>(&op)
           // EmitC ops.
           .Case<emitc::ApplyOp, emitc::CallOp, emitc::ConstantOp,