[ir] Deprecate `FrontendAtomicStmt` #907

k-ye · 2020-05-01T11:05:57Z

I added AtomicOpExpression back in Jan, where we wanted to make ti.atomic_*() return the old value. Now I think it's a good time to merge FrontendAtomicStmt into AtomicOpExpression.

Related issue = #689 , #332

[Click here for the format server]

archibate

Great step towards TAI/CHI IR standardization! LGTM except for a few questions..

archibate · 2020-05-01T13:14:59Z

taichi/ir/ir.cpp

+  // replace atomic sub with negative atomic add
+  if (op_type == AtomicOpType::sub) {
+    val.set(Expr::make<UnaryOpExpression>(UnaryOpType::neg, val));
+    op_type = AtomicOpType::add;
+  }


Why? Could you clarify the reason in the comment? I don't understand.. wasn't atomic_sub slimer than neg+atomic_add?

I didn't make any change here. Please compare this with the old code (visit(FrontendAtomicStmt*))

So we should ask @yuanming-hu for reason?

I think the reason is simply that the codegen assumes that sub is converted to add in the previous passes, see

taichi/taichi/codegen/codegen_llvm.cpp

Lines 947 to 963 in 75335e9

if (stmt->op_type == AtomicOpType::add) {

if (is_integral(stmt->val->ret_type.data_type)) {

old_value = builder->CreateAtomicRMW(

llvm::AtomicRMWInst::BinOp::Add, llvm_val[stmt->dest],

llvm_val[stmt->val], llvm::AtomicOrdering::SequentiallyConsistent);

} else if (stmt->val->ret_type.data_type == DataType::f32) {

old_value =

builder->CreateCall(get_runtime_function("atomic_add_f32"),

{llvm_val[stmt->dest], llvm_val[stmt->val]});

} else if (stmt->val->ret_type.data_type == DataType::f64) {

old_value =

builder->CreateCall(get_runtime_function("atomic_add_f64"),

{llvm_val[stmt->dest], llvm_val[stmt->val]});

} else {

TI_NOT_IMPLEMENTED

}

} else if (stmt->op_type == AtomicOpType::min) {

. It only handles add, but not sub

So let's just add handles for sub? It should have no harm but profitable right? Also metal and gl handles sub too, no reason special for llvm.

I think the current approach is fine. Adding handling forsub will have to cover all the backends, not just for LLVM. Again, let’s always focus one PR for one thing.

taichi/ir/expr.cpp

Co-authored-by: 彭于斌 <1931127624@qq.com>

archibate

Looks good in general, thank you!

archibate · 2020-05-02T04:36:01Z

taichi/ir/ir.cpp

+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));


Will this cause an circular reference?

Good point, but nope. If you look into set():

taichi/taichi/ir/expr.h

Lines 47 to 49 in f5373b1

void set(const Expr &o) {

expr = o.expr;

}

It assigns expr to another std::shared_ptr. shared_ptr::operator=() will ref the new object, and deref the old object. https://en.cppreference.com/w/cpp/memory/shared_ptr/operator%3D

archibate · 2020-05-02T04:38:33Z

taichi/ir/expr.cpp

-    current_ast_builder().insert(Stmt::make<FrontendAtomicStmt>(
-        AtomicOpType::add, *this, -load_if_ptr(o)));
+    (*this) = Expr::make<AtomicOpExpression>(
+        AtomicOpType::sub, ptr_if_global(*this), load_if_ptr(o));


Why ptr_if_global appeared? I thought it's already done in L856?

I think it's a bug in the old code. See L131 in the old code for how it handles ::add, which has ptr_if_global. Also L856 checks for GlobalPtrExpression, whereas ptr_if_global() checks for GlobalVariableExpression.

Actually, my feeling is ptr_if_global should be removed completely in a future PR. We use this wrapper just to allow the syntax like a += 1 for a = 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).

ah ok, then let's do that in a separate PR..

yuanming-hu

Thanks! LGTM.

yuanming-hu · 2020-05-02T05:56:05Z

taichi/ir/expr.cpp

-    current_ast_builder().insert(Stmt::make<FrontendAtomicStmt>(
-        AtomicOpType::add, *this, -load_if_ptr(o)));
+    (*this) = Expr::make<AtomicOpExpression>(
+        AtomicOpType::sub, ptr_if_global(*this), load_if_ptr(o));


Actually, my feeling is ptr_if_global should be removed completely in a future PR. We use this wrapper just to allow the syntax like a += 1 for a = 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).

[ir] Deprecate FrontendAtomicStmt

7d9ab56

k-ye requested a review from yuanming-hu May 1, 2020 11:06

archibate suggested changes May 1, 2020

View reviewed changes

k-ye and others added 2 commits May 1, 2020 23:22

Update taichi/ir/expr.cpp

61cb698

Co-authored-by: 彭于斌 <1931127624@qq.com>

[skip ci] Merge branch 'master' into atomics

ef997cc

archibate approved these changes May 2, 2020

View reviewed changes

yuanming-hu mentioned this pull request May 2, 2020

[ir] [refactor] Make Expr::operator=(const Expr &o) single responsibility #910

Open

yuanming-hu approved these changes May 2, 2020

View reviewed changes

k-ye merged commit d19ccd9 into taichi-dev:master May 2, 2020

k-ye deleted the atomics branch May 2, 2020 06:29

strongoier mentioned this pull request Oct 26, 2021

[ir] [refactor] Remove ptr_if_global in C++ Expr class #3285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ir] Deprecate `FrontendAtomicStmt` #907

[ir] Deprecate `FrontendAtomicStmt` #907

k-ye commented May 1, 2020

archibate left a comment

archibate May 1, 2020

k-ye May 1, 2020

archibate May 1, 2020

k-ye May 1, 2020

archibate May 1, 2020

k-ye May 1, 2020

archibate left a comment

archibate May 2, 2020

k-ye May 2, 2020

archibate May 2, 2020

k-ye May 2, 2020 •

edited

yuanming-hu May 2, 2020

k-ye May 2, 2020

yuanming-hu left a comment

yuanming-hu May 2, 2020

	if (stmt->op_type == AtomicOpType::add) {
	if (is_integral(stmt->val->ret_type.data_type)) {
	old_value = builder->CreateAtomicRMW(
	llvm::AtomicRMWInst::BinOp::Add, llvm_val[stmt->dest],
	llvm_val[stmt->val], llvm::AtomicOrdering::SequentiallyConsistent);
	} else if (stmt->val->ret_type.data_type == DataType::f32) {
	old_value =
	builder->CreateCall(get_runtime_function("atomic_add_f32"),
	{llvm_val[stmt->dest], llvm_val[stmt->val]});
	} else if (stmt->val->ret_type.data_type == DataType::f64) {
	old_value =
	builder->CreateCall(get_runtime_function("atomic_add_f64"),
	{llvm_val[stmt->dest], llvm_val[stmt->val]});
	} else {
	TI_NOT_IMPLEMENTED
	}
	} else if (stmt->op_type == AtomicOpType::min) {

[ir] Deprecate FrontendAtomicStmt #907

[ir] Deprecate FrontendAtomicStmt #907

Conversation

k-ye commented May 1, 2020

archibate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archibate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-ye May 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanming-hu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[ir] Deprecate `FrontendAtomicStmt` #907

[ir] Deprecate `FrontendAtomicStmt` #907

k-ye May 2, 2020 •

edited