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

[opt] Slightly simplify algebraic simplification #2337

Merged
merged 5 commits into from
May 18, 2021

Conversation

xumingkuan
Copy link
Contributor

Related issue = a continuation of #2332

val_cast_to_uint64() looks cleaner than two if-branches with the same body.

[Click here for the format server]


@xumingkuan xumingkuan requested a review from k-ye May 13, 2021 06:57
@xumingkuan
Copy link
Contributor Author

I'm not sure if we need a function to cast integer values to int64/uint64 (i.e., handle signed/unsigned cases together)...

@k-ye
Copy link
Member

k-ye commented May 15, 2021

I'm not sure if we need a function to cast integer values to int64/uint64 (i.e., handle signed/unsigned cases together)...

We already have val_int() and val_uint()?

We do need to add the const version of these functions. For now they return a mutable reference to the underlying data..

@xumingkuan
Copy link
Contributor Author

I'm not sure if we need a function to cast integer values to int64/uint64 (i.e., handle signed/unsigned cases together)...

We already have val_int() and val_uint()?

These 2 functions only handles signed/unsigned versions separately. I wonder if we want to use only one function when we don't care if the value of the TypedConstant is signed or not.

We do need to add the const version of these functions. For now they return a mutable reference to the underlying data..

They are already const, I think.

@k-ye
Copy link
Member

k-ye commented May 15, 2021

I'm not sure if we need a function to cast integer values to int64/uint64 (i.e., handle signed/unsigned cases together)...

We already have val_int() and val_uint()?

These 2 functions only handles signed/unsigned versions separately. I wonder if we want to use only one function when we don't care if the value of the TypedConstant is signed or not.

Right, sounds like it could be useful

We do need to add the const version of these functions. For now they return a mutable reference to the underlying data..

They are already const, I think.

Ah, i was talking about these:

taichi/taichi/ir/type.h

Lines 402 to 411 in c824a8b

int32 &val_int32();
float32 &val_float32();
int64 &val_int64();
float64 &val_float64();
int8 &val_int8();
int16 &val_int16();
uint8 &val_uint8();
uint16 &val_uint16();
uint32 &val_uint32();
uint64 &val_uint64();

@xumingkuan
Copy link
Contributor Author

We do need to add the const version of these functions. For now they return a mutable reference to the underlying data..

They are already const, I think.

Ah, i was talking about these:

taichi/taichi/ir/type.h

Lines 402 to 411 in c824a8b

int32 &val_int32();
float32 &val_float32();
int64 &val_int64();
float64 &val_float64();
int8 &val_int8();
int16 &val_int16();
uint8 &val_uint8();
uint16 &val_uint16();
uint32 &val_uint32();
uint64 &val_uint64();

Right, I agree that we need these, but maybe copying these functions will make this file too long?

@k-ye
Copy link
Member

k-ye commented May 17, 2021

Right, I agree that we need these, but maybe copying these functions will make this file too long?

That shouldn't be a concern...

@xumingkuan
Copy link
Contributor Author

This PR should be ready for review now :)

@xumingkuan xumingkuan merged commit 375dd4a into taichi-dev:master May 18, 2021
@xumingkuan xumingkuan mentioned this pull request May 20, 2021
@xumingkuan xumingkuan deleted the pot branch June 8, 2021 06:55
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.

2 participants