Skip to content

[TIR][BugFix] Do not bind non-index type value of lets in CompactBufferAllocation#11828

Merged
junrushao merged 1 commit intoapache:mainfrom
wrongtest-intellif:fix_let_binding_in_compact_buffer_pass
Jun 28, 2022
Merged

[TIR][BugFix] Do not bind non-index type value of lets in CompactBufferAllocation#11828
junrushao merged 1 commit intoapache:mainfrom
wrongtest-intellif:fix_let_binding_in_compact_buffer_pass

Conversation

@wrongtest-intellif
Copy link
Contributor

Fix a bug for let support, it should not put non-index typed values into dom_map.

@junrushao
Copy link
Member

Hey thanks for the contribution! I don't think I fully comprehend this change (and the unittest) so would love to hear more about your insights :-) With this change, whether or not to add a tir::Var into dom_map is based on its dtype. Why is it the case?

@wrongtest-intellif
Copy link
Contributor Author

Hey thanks for the contribution! I don't think I fully comprehend this change (and the unittest) so would love to hear more about your insights :-) With this change, whether or not to add a tir::Var into dom_map is based on its dtype. Why is it the case?

Emmmmm,my understand is that since the pass is only interested in buffer indices,record index typed vars could be safe. While arith utilities are written to handle integers,binding things like SinglePoint(int32x8) would just fail on sanity checks of dom analysis (eg, EvalNDSet).

@junrushao
Copy link
Member

Agree that we shouldn’t bind vectorized data types. Besides that, is that anything else we shouldn’t bind? If not, we can instead only check lanes field

@junrushao
Copy link
Member

Perhaps we can add an assertion in the Bind method where we always assume the value shouldn’t be vectorized

@wrongtest-intellif
Copy link
Contributor Author

wrongtest-intellif commented Jun 23, 2022

is that anything else we shouldn’t bind

And kVoid and kHandle should not bind.

  • kVoid: lanes=0
  • kHandle: lanes can be arbitrary

IsIndexType also excludes

  • float, uint: bind them into dom_map or into analyzer seems cause no error, but maybe effectless in integer analysis about buffer indices.

@wrongtest-intellif
Copy link
Contributor Author

Perhaps we can add an assertion in the Bind method where we always assume the value shouldn’t be vectorized

Thanks for the notes~ I'd like to add one ICHECK line to see if there are broken cases. For vectors, however, I'm not sure it should never be considered to get analyzed/simplified. For CompactBufferAllocation, just integer analysis part (IntSets) should not work with vectors.

@junrushao
Copy link
Member

@wrongtest Thank you for your detailed explanation! That makes a lot of sense to me :-)

@junrushao
Copy link
Member

Looks like some situations happened in the CI...

@junrushao
Copy link
Member

@wrongtest would you mind retriggering the CI? looks like it's down

@wrongtest-intellif wrongtest-intellif force-pushed the fix_let_binding_in_compact_buffer_pass branch 2 times, most recently from 1159b9f to 5ea7b52 Compare June 25, 2022 14:32
@wrongtest-intellif wrongtest-intellif force-pushed the fix_let_binding_in_compact_buffer_pass branch from 5ea7b52 to 971db0e Compare June 26, 2022 06:41
@junrushao
Copy link
Member

Retriggering as CI is back to life

@junrushao junrushao merged commit d4be49a into apache:main Jun 28, 2022
blackkker pushed a commit to blackkker/tvm that referenced this pull request Jul 7, 2022
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
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