Page MenuHomePhabricator

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

Authored by xndcn on Tue, Sep 14, 7:43 PM.

Details

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;

Diff Detail

Event Timeline

xndcn created this revision.Tue, Sep 14, 7:43 PM
xndcn requested review of this revision.Tue, Sep 14, 7:43 PM

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

mehdi_amini added inline comments.Tue, Sep 14, 8:03 PM
mlir/lib/Target/Cpp/TranslateToCpp.cpp
879

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

xndcn edited the summary of this revision. (Show Details)Tue, Sep 14, 8:11 PM
xndcn edited the summary of this revision. (Show Details)
xndcn updated this revision to Diff 372623.Tue, Sep 14, 8:15 PM
xndcn marked an inline comment as done.
xndcn added inline comments.
mlir/lib/Target/Cpp/TranslateToCpp.cpp
879

Using getOStream() to ignore indent for block label

marbre accepted this revision.Wed, Sep 15, 11:26 AM

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.

This revision is now accepted and ready to land.Wed, Sep 15, 11:26 AM
jpienaar accepted this revision.Thu, Sep 16, 6:40 AM
jpienaar added inline comments.
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 added a comment.Thu, Sep 16, 8:14 AM

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;
  };
This revision was automatically updated to reflect the committed changes.
xndcn added a comment.Sun, Sep 19, 5:09 AM

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";