-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ir] Deprecate FrontendAtomicStmt
#907
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -553,12 +553,6 @@ FrontendAssignStmt::FrontendAssignStmt(const Expr &lhs, const Expr &rhs) | |||||||||||||||||||||||||||||||||||
TI_ASSERT(lhs->is_lvalue()); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
FrontendAtomicStmt::FrontendAtomicStmt(AtomicOpType op_type, | ||||||||||||||||||||||||||||||||||||
const Expr &dest, | ||||||||||||||||||||||||||||||||||||
const Expr &val) | ||||||||||||||||||||||||||||||||||||
: op_type(op_type), dest(dest), val(val) { | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
IRNode *FrontendContext::root() { | ||||||||||||||||||||||||||||||||||||
return static_cast<IRNode *>(root_node.get()); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
@@ -845,6 +839,28 @@ std::string AtomicOpExpression::serialize() { | |||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
void AtomicOpExpression::flatten(FlattenContext *ctx) { | ||||||||||||||||||||||||||||||||||||
// replace atomic sub with negative atomic add | ||||||||||||||||||||||||||||||||||||
if (op_type == AtomicOpType::sub) { | ||||||||||||||||||||||||||||||||||||
val.set(Expr::make<UnaryOpExpression>(UnaryOpType::neg, val)); | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this cause an circular reference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, but nope. If you look into Lines 47 to 49 in f5373b1
It assigns |
||||||||||||||||||||||||||||||||||||
op_type = AtomicOpType::add; | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
Comment on lines
+843
to
+847
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? Could you clarify the reason in the comment? I don't understand.. wasn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't make any change here. Please compare this with the old code ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we should ask @yuanming-hu for reason? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the reason is simply that the codegen assumes that taichi/taichi/codegen/codegen_llvm.cpp Lines 947 to 963 in 75335e9
add , but not sub
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So let's just add handles for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the current approach is fine. Adding handling for |
||||||||||||||||||||||||||||||||||||
// expand rhs | ||||||||||||||||||||||||||||||||||||
auto expr = val; | ||||||||||||||||||||||||||||||||||||
expr->flatten(ctx); | ||||||||||||||||||||||||||||||||||||
if (dest.is<IdExpression>()) { // local variable | ||||||||||||||||||||||||||||||||||||
// emit local store stmt | ||||||||||||||||||||||||||||||||||||
auto alloca = ctx->current_block->lookup_var(dest.cast<IdExpression>()->id); | ||||||||||||||||||||||||||||||||||||
ctx->push_back<AtomicOpStmt>(op_type, alloca, expr->stmt); | ||||||||||||||||||||||||||||||||||||
} else { // global variable | ||||||||||||||||||||||||||||||||||||
TI_ASSERT(dest.is<GlobalPtrExpression>()); | ||||||||||||||||||||||||||||||||||||
auto global_ptr = dest.cast<GlobalPtrExpression>(); | ||||||||||||||||||||||||||||||||||||
global_ptr->flatten(ctx); | ||||||||||||||||||||||||||||||||||||
ctx->push_back<AtomicOpStmt>(op_type, ctx->back_stmt(), expr->stmt); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
stmt = ctx->back_stmt(); | ||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
std::string SNodeOpExpression::serialize() { | ||||||||||||||||||||||||||||||||||||
if (value.expr) { | ||||||||||||||||||||||||||||||||||||
return fmt::format("{}({}, [{}], {})", snode_op_type_name(op_type), | ||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
ptr_if_global
appeared? I thought it's already done in L856?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a bug in the old code. See L131 in the old code for how it handles
::add
, which hasptr_if_global
. Also L856 checks for GlobalPtrExpression, whereasptr_if_global()
checks for GlobalVariableExpression.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, my feeling is
ptr_if_global
should be removed completely in a future PR. We use this wrapper just to allow the syntax likea += 1
fora = ti.var(ti.f32, shape())
(0-D tensors). Enforcing the user to write ``a[None] += 1` will make learning easier since the rule is simpler (though more verbose).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, then let's do that in a separate PR..