Remove duplicate as Checks and CHECK value #2531
Conversation
src/pass/verify_gpu_code.cc
Outdated
| @@ -73,9 +73,10 @@ class GPUCodeVerifier : public IRVisitor { | |||
|
|
|||
| void Visit_(const AttrStmt *op) { | |||
| if (op->attr_key == attr::storage_scope) { | |||
| if (op->value.as<StringImm>()->value == "local") { | |||
| std::string opValue = op->value.as<StringImm>()->value; | |||
There was a problem hiding this comment.
naming convention: opValue->op_value
There was a problem hiding this comment.
Fixed (the next one too).
src/schedule/graph.cc
Outdated
| if (op.as<ScanOpNode>()) { | ||
| const auto& update = op.as<ScanOpNode>()->update; | ||
| const auto& init = op.as<ScanOpNode>()->init; | ||
| if (auto scanOp = op.as<ScanOpNode>()) { |
| @@ -470,8 +470,8 @@ void CodeGenStackVM::VisitExpr_(const Select *op) { | |||
| } | |||
|
|
|||
| void CodeGenStackVM::VisitStmt_(const AssertStmt *op) { | |||
| if (op->message.as<StringImm>()) { | |||
| int sid = this->GetStrID(op->message.as<StringImm>()->value); | |||
| if (auto str = op->message.as<StringImm>()) { | |||
There was a problem hiding this comment.
Make this "const auto* str" instead of "auto str", to expressly show that str is expected to be a const pointer.
There was a problem hiding this comment.
The "as" template is defined here. Very hard to dig out:
https://github.com/dmlc/HalideIR/blob/master/src/tvm/node/node.h#L306
alexeyr
force-pushed
the
pvs-warnings
branch
3 times, most recently
from
3cf47f4
to
8743399
Compare
|
@tqchen Given the name change, would it be better to split first two commits into a separate pull request? |
alexeyr
force-pushed
the
pvs-warnings
branch
2 times, most recently
from
6ba5a6a
to
f0110ae
Compare
| @@ -718,10 +718,10 @@ class StoragePlanRewriter : public IRMutator { | |||
| src_entry->attach_scope_ == thread_scope_ && | |||
| src_entry->elem_type == ae.alloc->type.element_of() && | |||
| visitor.Check(s.stmt, var, src)) { | |||
| uint64_t const_nbits = static_cast<uint64_t>( | |||
There was a problem hiding this comment.
This change is non-necessary, and makes the code worse. It is best to do the multiplication in uint32_t and then cast the result to uint64_t, than doing the math in uint64_t, which is more expensive. Hypothetically when multiply 32 bits integers the result could overflow 32 bits range. But first it is not going to happen. Second if it happens you have a bigger problem as you cannot allocate 4GB (2^32) worth of memory at a time. So don't change here.
There was a problem hiding this comment.
Yes, I was thinking about an overflow. But your second point especially makes sense, I'll remove it.
There was a problem hiding this comment.
Wait, it's nbits, not bytes, so it's "only" 536 MB (that's probably why it's uint64_t and not size_t in the first place). Still, the multipliers are probably small enough, as your first point mentions.
|
it is fine, given these changes are all minor fixes, we can merge this in as a single PR, please address the CI problem and comments |
|
@tqchen I will, thanks. |
tqchen
added
status: accepted
and removed
status: need update
|
Thanks to @alexeyr @Anthony-Mai , this is now merged |
The first two commits fix a few problems found by PVS Studio, the third removes some duplicate
as<Whatever>calls for readability and consistency.