[ir] Add a function to test if two IRNodes are equivalent

[xumingkuan]

Related issue = #656 (comment)

[Click here for the format server]

[xumingkuan]

Shall we move

taichi/taichi/ir/ir.h

Line 577 in e8e42f3

bool is() const {

to IRNode in order to test if IRNode *other is of the same type with the current IRNode *? (for example, to test if other is Block * in void visit(Block *stmt_list))

[yuanming-hu]

Good idea!

[xumingkuan]

I was writing the IRNode comparator while referring to the IR printer to see which fields each kind of Stmts has. I noticed that many Frontend...Stmt's visit function of IR printer invokes serialize, of which it would be extremely hard to compare if two are the same without re_id.

I wonder if, in the stage of calling our IRNode comparator, there would be no Frontend...Stmt's? If the answer is yes, I would be able to build an std::map to incrementally store the relationships of ids.

[yuanming-hu]

I wonder if, in the stage of calling our IRNode comparator, there would be no Frontend...Stmt's? If the answer is yes, I would be able to build an std::map to incrementally store the relationships of ids.

Yes, you can assume you are working on a lowered AST.

[xumingkuan]

Is type_hint() a necessary field to check?

[yuanming-hu]

Yes, we should assume this process happens only after type_check, where ret_type of all statements are computed.

[yuanming-hu]

We may need to use the std::unordered_ versions for efficienty.

[archibate]

What's the advantage of map? If unordered_map always superior, then why they don't replace it then?

[xumingkuan]

std::map uses less memory, provides functions about the order (lower_bound for example), and guarantees worst-case O(log(n)) time (std::unordered_map has a worst-case complexity of O(n)). I agree that we should use std::unordered_map in this case.

[xumingkuan]

I just noticed that the indices inserted to id_map must be in ascending order. Maybe std::vector is more efficient if the range is not too big (and after re_id, it is not). However, I would still prefer to use std::unordered_map for maintainable code.

[xumingkuan]

Why are there XStmts in both ir.h and statements.h?

[xumingkuan]

Well... In some cases, a SNode * could be a nullptr. Maybe it's also OK to check if two SNode *'s (pointers) are the same, rather than check their id's?

[yuanming-hu]

Well... In some cases, a SNode * could be a nullptr. Maybe it's also OK to check if two SNode *'s (pointers) are the same, rather than check their id's?

Yes, you can simply use the pointers for comparison. LLVM uses that as well.

Or you can define a function that takes in SNode * and returns the unique_ids and -1 on null pointers.

[yuanming-hu]

Well... In some cases, a SNode * could be a nullptr. Maybe it's also OK to check if two SNode *'s (pointers) are the same, rather than check their id's?

Yes, you can simply use the pointers for comparison. LLVM uses that as well.

Or you can define a function that takes in SNode * and returns the unique_ids and -1 on null pointers.

Let's take the second approach, considering we may allow deleting/adding new SNodes dynamically and reuse pointer addresses in the future. LLVM uses pointer comparison on its types because it assumes once created the types won't get recycled.

[xumingkuan]

Or you can define a function that takes in SNode * and returns the unique_ids and -1 on null pointers.

It can't be applied directly with this macro though...

#define DEFINE_FIELD_CHECK(field)    \
  if (stmt->field != other->field) { \
    same = false;                    \
    return;                          \
  }

Shall I write another macro like this?

#define DEFINE_SNODE_CHECK(snode)    \
  if (get_snode_id(stmt->snode) != get_snode_id(other->snode)) { \
    same = false;                    \
    return;                          \
  }

[xumingkuan]

Seems that

taichi/taichi/ir/statements.h

Line 179 in a742a9f

int step;

and

taichi/taichi/ir/statements.h

Line 184 in a742a9f

std::vector<Stmt *> loop_vars;

are never used?

[xumingkuan]

Why is Travis CI failing?

[xumingkuan]

I see. Probably because LocalAddress is used before definition...

[yuanming-hu]

Awesome!

[yuanming-hu]

One final thing before merging this: could you add a pass after lower_ast to check if fields_registered is true for all statements? Thanks!

[yuanming-hu]

I realized type_check is a better place to check these fields. I also registered WhileStmt. I'll merge this in once CI passes. Thanks!

[xumingkuan]

WhileStmt::mask was not an operand. What does it do and what's the effect of adding it to operands?

[yuanming-hu]

#583 (comment)

[xumingkuan]

Should WhileStmt::mask be similar to WhileControlStmt::mask, what about IfStmt::true_mask and IfStmt::false_mask? Just to make sure everything is good as IfStmt::true_mask and IfStmt::false_mask are not operands.

[yuanming-hu]

Should WhileStmt::mask be similar to WhileControlStmt::mask, what about IfStmt::true_mask and IfStmt::false_mask? Just to make sure everything is good as IfStmt::true_mask and IfStmt::false_mask are not operands.

Maybe I should redefine operand: they are defined as all Stmt * fields of a statement. Let's initialize all the remaining operands (especially the masks) to nullptr and register them. (I realized that for IfStmt, true/false mask are not registered & initialized...)

[yuanming-hu]

LGTM now. Thank you so much, Mingkuan!