[type] [refactor] Add compute_type for CustomIntType #2047

Hanke98 · 2020-11-13T04:11:22Z

Related issue = #1905

In this pr, we add an attribute for CustomIntType named 'compute_type' and replace some 'hand-write 32'. Some TODO could be removed after this pr being merged.

[Click here for the format server]

codecov · 2020-11-13T04:51:28Z

Codecov Report

Merging #2047 (985f35e) into master (46d5801) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2047      +/-   ##
==========================================
+ Coverage   43.54%   43.56%   +0.01%     
==========================================
  Files          45       45              
  Lines        6267     6267              
  Branches     1110     1110              
==========================================
+ Hits         2729     2730       +1     
  Misses       3366     3366              
+ Partials      172      171       -1

Impacted Files	Coverage Δ
python/taichi/lang/transformer.py	`81.32% <0.00%> (-0.70%)`	⬇️
python/taichi/lang/impl.py	`66.93% <0.00%> (+0.53%)`	⬆️
python/taichi/lang/snode.py	`68.33% <0.00%> (+2.50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c90be8a...985f35e. Read the comment docs.

TH3CHARLie

Looks good, thanks!

TH3CHARLie · 2020-11-13T07:05:33Z

taichi/ir/type.h

+    //  and should we expose it to users?
+    TI_ASSERT(num_bits <= 32);
+    compute_type = is_signed ? new PrimitiveType(PrimitiveTypeID::i32)
+                             : new PrimitiveType(PrimitiveTypeID::u32);


Maybe use type_factory here?

Hanke98 · 2020-11-14T07:05:17Z

Please don't merge this pr for now. I have found some potential problems and will request your review after I fixed them. Thanks!

Hanke98 · 2020-11-14T10:39:30Z

taichi/codegen/codegen_llvm.cpp

+    auto left =
+        builder->CreateSub(tlctx->get_constant(compute_type_size), bit_end);
+    auto right = builder->CreateSub(tlctx->get_constant(compute_type_size),
+                                    tlctx->get_constant(cit->get_num_bits()));
+    left = builder->CreateIntCast(left, llvm_type(cit->get_compute_type()),
+                                  cit->get_is_signed());
+    right = builder->CreateIntCast(right, llvm_type(cit->get_compute_type()),


After testing the case of 16-bits and 64-bits, I found that we should use the num of physical_type bits in BitStructType SNode here instead of the compute_type of CustomIntType. So I decided to convert this pr to draft and reimplement this part.

yuanming-hu

Cool! I didn't go through every file but it looks that we are on the right track!

yuanming-hu · 2020-11-17T04:28:46Z

taichi/codegen/codegen_llvm.cpp

+llvm::Type *CodeGenLLVM::llvm_ptr_type(DataType dt) {
+  if (dt->is_primitive(PrimitiveTypeID::i8) ||
+      dt->is_primitive(PrimitiveTypeID::u8)) {
+    return llvm::Type::getInt8PtrTy(*llvm_context);
+  } else if (dt->is_primitive(PrimitiveTypeID::i16) ||
+             dt->is_primitive(PrimitiveTypeID::u16)) {
+    return llvm::Type::getInt16PtrTy(*llvm_context);
+  } else if (dt->is_primitive(PrimitiveTypeID::i32) ||
+             dt->is_primitive(PrimitiveTypeID::u32)) {
+    return llvm::Type::getInt32PtrTy(*llvm_context);
+  } else if (dt->is_primitive(PrimitiveTypeID::i64) ||
+             dt->is_primitive(PrimitiveTypeID::u64)) {
+    return llvm::Type::getInt64PtrTy(*llvm_context);
+  } else if (dt->is_primitive(PrimitiveTypeID::u1)) {
+    return llvm::Type::getInt1PtrTy(*llvm_context);
+  } else if (dt->is_primitive(PrimitiveTypeID::f32)) {
+    return llvm::Type::getFloatPtrTy(*llvm_context);
+  } else if (dt->is_primitive(PrimitiveTypeID::f64)) {
+    return llvm::Type::getDoublePtrTy(*llvm_context);
+  } else {
+    TI_NOT_IMPLEMENTED;
+  }
+  return nullptr;
+}


Suggested change

llvm::Type *CodeGenLLVM::llvm_ptr_type(DataType dt) {

if (dt->is_primitive(PrimitiveTypeID::i8) ||

dt->is_primitive(PrimitiveTypeID::u8)) {

return llvm::Type::getInt8PtrTy(*llvm_context);

} else if (dt->is_primitive(PrimitiveTypeID::i16) ||

dt->is_primitive(PrimitiveTypeID::u16)) {

return llvm::Type::getInt16PtrTy(*llvm_context);

} else if (dt->is_primitive(PrimitiveTypeID::i32) ||

dt->is_primitive(PrimitiveTypeID::u32)) {

return llvm::Type::getInt32PtrTy(*llvm_context);

} else if (dt->is_primitive(PrimitiveTypeID::i64) ||

dt->is_primitive(PrimitiveTypeID::u64)) {

return llvm::Type::getInt64PtrTy(*llvm_context);

} else if (dt->is_primitive(PrimitiveTypeID::u1)) {

return llvm::Type::getInt1PtrTy(*llvm_context);

} else if (dt->is_primitive(PrimitiveTypeID::f32)) {

return llvm::Type::getFloatPtrTy(*llvm_context);

} else if (dt->is_primitive(PrimitiveTypeID::f64)) {

return llvm::Type::getDoublePtrTy(*llvm_context);

} else {

TI_NOT_IMPLEMENTED;

}

return nullptr;

}

llvm::Type *CodeGenLLVM::llvm_ptr_type(DataType dt) {

return llvm::PointerType::get(llvm_type(dt), 0);

}

taichi/codegen/codegen_llvm.cpp

taichi/ir/type.cpp

Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>

Hanke98

Finally, I think it's time for a formal review. Please help me to check my refactoring! Thanks a lot !

taichi/codegen/codegen_llvm.cpp

Hanke98 · 2020-11-19T13:24:31Z

taichi/ir/type_factory.h

+  Type *get_custom_int_type_with_compute_type(int compute_type_bits,
+                                              int num_bits,
+                                              bool is_signed);


The function name might be too long. Any good advices would be appreciated!

My two cents: actually every CustomIntType has a compute type. I would use get_custom_int_type(int num_bits, bool is_signed, Type *compute_type=nullptr) if that makes sense to you. Then two APIs can be merged into one.

yuanming-hu

Very cool! Feel free to merge after the comments are addressed. Thanks!

taichi/ir/type.cpp

yuanming-hu · 2020-11-19T22:31:16Z

taichi/ir/type.h

+  CustomIntType(int num_bits, bool is_signed);
+
+  CustomIntType(int compute_type_bits, int numBits, bool isSigned);
+
+  CustomIntType(int compute_type_bits,
+                Type *physical_type,
+                int num_bits,
+                bool is_signed);


A single ctor CustomIntType(int num_bits, bool is_signed, Type *physical_type_=nullptr, Type *compute_type_=nullptr) may be cleaner?

yuanming-hu · 2020-11-19T22:33:28Z

taichi/ir/type_factory.h

+  Type *get_custom_int_type_with_compute_type(int compute_type_bits,
+                                              int num_bits,
+                                              bool is_signed);


My two cents: actually every CustomIntType has a compute type. I would use get_custom_int_type(int num_bits, bool is_signed, Type *compute_type=nullptr) if that makes sense to you. Then two APIs can be merged into one.

Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>

TH3CHARLie

LGTM! Just one minor comment improvement

TH3CHARLie · 2020-11-19T13:41:05Z

taichi/codegen/codegen_llvm.cpp

@@ -328,7 +328,8 @@ void CodeGenLLVM::visit(UnaryOpStmt *stmt) {
      auto from_size = 0;
      if (from->is<CustomIntType>()) {
        // TODO: replace 32 with a customizable type


remove TODO here

Hanke98 added 3 commits November 12, 2020 20:45

rebase code from upstream

41b9965

add llvm_ptr_type and data type bits

0b8fd8b

add some comments

b1b5e56

Hanke98 requested review from TH3CHARLie, yuanming-hu and taichi-gardener November 13, 2020 04:11

[skip ci] enforce code format

100ea35

TH3CHARLie approved these changes Nov 13, 2020

View reviewed changes

Hanke98 and others added 4 commits November 13, 2020 16:35

use type factory

3399981

[skip ci] enforce code format

e98f450

update runtime

9ff8476

[skip ci] enforce code format

04cb2f3

Hanke98 commented Nov 14, 2020

View reviewed changes

Hanke98 marked this pull request as draft November 14, 2020 10:41

Hanke98 and others added 10 commits November 15, 2020 18:33

modify bit_ptr struct and rebase

15ffdfa

[skip ci] enforce code format

a44bd21

fix bit_array

6d82d39

use runtime to do global loading

fab966e

[skip ci] enforce code format

7bfbcb5

add tests

1f9e511

[skip ci] enforce code format

9b109f5

add more tetst cases and rebase

10cd722

[skip ci] enforce code format

45dabf8

fix bit struct

9d99341

yuanming-hu reviewed Nov 18, 2020

View reviewed changes

Hanke98 and others added 3 commits November 19, 2020 09:59

set physical type for bit_array

f045ec1

[skip ci] enforce code format

e344ca0

modify test cases

a20a0e8

taichi-gardener and others added 5 commits November 19, 2020 02:07

[skip ci] enforce code format

18a6738

Apply suggestions from code review

b8c4d49

Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>

fix type factory

a7acc57

change funtion name

704bbe7

[skip ci] enforce code format

5022a10

Hanke98 commented Nov 19, 2020

View reviewed changes

Hanke98 requested review from yuanming-hu and TH3CHARLie November 19, 2020 13:30

Hanke98 marked this pull request as ready for review November 19, 2020 13:30

yuanming-hu approved these changes Nov 19, 2020

View reviewed changes

Hanke98 and others added 5 commits November 20, 2020 09:52

Apply suggestions from code review

c1bf0e6

Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>

[skip ci] enforce code format

cc8878b

modify APIs

a2d4196

[skip ci] enforce code format

c9993cf

update

29f68ad

TH3CHARLie approved these changes Nov 20, 2020

View reviewed changes

remove todo

985f35e

Hanke98 merged commit 2b9b0a2 into taichi-dev:master Nov 20, 2020

yuanming-hu mentioned this pull request Nov 20, 2020

[release] v0.7.8 #2058

Merged

Hanke98 deleted the impl-compute-type branch November 27, 2020 06:32

Hanke98 mentioned this pull request Jun 2, 2021

[type] [bug] Remove redundant component of bit pointer struct #2393

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[type] [refactor] Add compute_type for CustomIntType #2047

[type] [refactor] Add compute_type for CustomIntType #2047

Hanke98 commented Nov 13, 2020

codecov bot commented Nov 13, 2020 •

edited

TH3CHARLie left a comment

TH3CHARLie Nov 13, 2020

Hanke98 commented Nov 14, 2020

Hanke98 Nov 14, 2020

yuanming-hu left a comment

yuanming-hu Nov 17, 2020 •

edited

Hanke98 left a comment

Hanke98 Nov 19, 2020

yuanming-hu Nov 19, 2020

yuanming-hu left a comment

yuanming-hu Nov 19, 2020

yuanming-hu Nov 19, 2020

TH3CHARLie left a comment

TH3CHARLie Nov 19, 2020

[type] [refactor] Add compute_type for CustomIntType #2047

[type] [refactor] Add compute_type for CustomIntType #2047

Conversation

Hanke98 commented Nov 13, 2020

codecov bot commented Nov 13, 2020 • edited

Codecov Report

TH3CHARLie left a comment

Choose a reason for hiding this comment

TH3CHARLie Nov 13, 2020

Choose a reason for hiding this comment

Hanke98 commented Nov 14, 2020

Hanke98 Nov 14, 2020

Choose a reason for hiding this comment

yuanming-hu left a comment

Choose a reason for hiding this comment

yuanming-hu Nov 17, 2020 • edited

Choose a reason for hiding this comment

Hanke98 left a comment

Choose a reason for hiding this comment

Hanke98 Nov 19, 2020

Choose a reason for hiding this comment

yuanming-hu Nov 19, 2020

Choose a reason for hiding this comment

yuanming-hu left a comment

Choose a reason for hiding this comment

yuanming-hu Nov 19, 2020

Choose a reason for hiding this comment

yuanming-hu Nov 19, 2020

Choose a reason for hiding this comment

TH3CHARLie left a comment

Choose a reason for hiding this comment

TH3CHARLie Nov 19, 2020

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2020 •

edited

yuanming-hu Nov 17, 2020 •

edited