Skip to content
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

[type] Support bit-level read and write in Python-scope #2029

Merged

Conversation

Hanke98
Copy link
Contributor

@Hanke98 Hanke98 commented Nov 6, 2020

Related issue = #1905 #1996

In this pr, we tried to support reading/writing bit_struct in python scope.
The key change is : support CustomIntType in get_data_type, get_ret_int and get_ret_int

[Click here for the format server]


@yuanming-hu yuanming-hu changed the title [type] Support bit-level read and write in python scope [type] Support bit-level read and write in Python-scope Nov 6, 2020
@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #2029 (3d63ba2) into master (3c48f4f) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2029      +/-   ##
==========================================
+ Coverage   43.52%   43.54%   +0.02%     
==========================================
  Files          45       45              
  Lines        6270     6267       -3     
  Branches     1110     1110              
==========================================
  Hits         2729     2729              
+ Misses       3369     3366       -3     
  Partials      172      172              
Impacted Files Coverage Δ
python/taichi/lang/__init__.py 40.41% <0.00%> (-0.17%) ⬇️
python/taichi/diagnose.py 0.00% <0.00%> (ø)

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 3c48f4f...e122c02. Read the comment docs.

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This mostly LGTM. Feel free to merge after the comments are resolved. TaichiLLVMContext::get_data_type should not be modified.

tests/python/test_bit_struct.py Outdated Show resolved Hide resolved
tests/python/test_bit_struct.py Outdated Show resolved Hide resolved
Comment on lines 304 to 308
} else if (auto cit = dt->cast<CustomIntType>()) {
if (cit->get_is_signed())
return (int64)get_current_program().fetch_result<int32>(i);
else
return (uint64)get_current_program().fetch_result<uint32>(i);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are good for now, but sometime in the future we may need to use i64.

Comment on lines 110 to 111
} else if (dt->is<CustomIntType>()) {
return llvm::Type::getInt32Ty(*ctx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this branch belongs here. CustomIntType should not be exposed to LLVM. It makes more sense to do the branching on the caller side of this function.

Copy link
Collaborator

@TH3CHARLie TH3CHARLie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and I verified @yuanming-hu's suggestions have been applied.

@Hanke98 Hanke98 merged commit 36232a5 into taichi-dev:master Nov 7, 2020
@yuanming-hu yuanming-hu mentioned this pull request Nov 7, 2020
@Hanke98 Hanke98 deleted the impl-bit-level-snode-read-and-write branch November 27, 2020 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants