[relay][refactor] Cache Op::Get in passes to reduce lookup overhead

[zhiics]

As @masahi mentioned a couple of times that it's necessary to introduce the "IsOp" helper instead of calling Op::Get everywhere, this PR refactors many places in the code to call this helper.

@masahi @icemelon9 @jroesch @tqchen @wweic @comaniac

[wweic]

lgtm. Could you add a unit test to make sure IsOp returns false for non op node?

[zhiics]

@wweic Yeah, I am aware of this. There are actually many cases already, like the ones in src/relay/pass/partial_eval.cc are not necessarily ops.

[tqchen]

Note, if we are concerned about efficiency, I still think we should separate Op::Get from checking. Note that we can always cache an op by storing it in a static var, or a class once and use that op to do equivalence checking.

Perhaps we should document this pattern and move away from getting op everytime

[zhiics]

@tqchen ops are already cached in registry, right? Op::Get asserts its existence and retrieves it from the cached entries.

[tqchen]

But that requires a hashtable lookup of string, while if you cache it locally in a pass, it is just a memory load

[comaniac]

Just carious. How large is the overhead of looking up the hashtable in this case compared to memory loading?

[tqchen]

Loading the op is just one instruction(think of generic llvm instr), while in the case of hashtable, we need first to create the string, hash it(O(len(str)), then lookup. While in terms of time complexity it is going to be the same if string's length is small, but it would mean quite a lot more instructions compared to he load.

if the function is in the innermost loop, then we could benefit from pre-cache the op(note that we can do that in Pass construction time).

[zhiics]

Okay. Thanks for discussion. I think we can just create some static vars in the passes that need specific Op::Get("xx") for equivalence checking.

[tqchen]

If the code is in a class, perhaps initializing these ops at the class construction(instead of static var) would be a better solution. This way we reduce the amount of static vars. @zhiics perhaps we could update the PR to document these recommendations?

[zhiics]

@tqchen Updated the PR. It looks the CI is down.

[masahi]

@tqchen do you want to take a final look?

[masahi]

@zhiics can you update the PR title ? I dont see any IsOp related change.

[zhiics]

@masahi Good point. Done. Thanks.

[masahi]

Thanks @zhiics @comaniac @wweic @tqchen