search

tlc-pack / relax

Apache License 2.0
193 stars 58 forks source link

Disallow inline prim_func in relax IR #414

Closed yongwww closed 1 year ago

yongwww commented 1 year ago

fix for #304

yongwww commented 1 year ago

Hi @tqchen @YuchenJin My initial understanding about WellFormed check here is that we need to ensure VarBinding’s value is not a PrimFunc. Seems PrimFunc may appear in many other places, such as CallNode’s args, do we need to check all of them?

MasterJH5574 commented 1 year ago

My initial understanding about WellFormed check here is that we need to ensure VarBinding’s value is not a PrimFunc. Seems PrimFunc may appear in many other places, such as CallNode’s args, do we need to check all of them?

When there’s a PrimFunc being a Call argument, the IR does not comply to normal form. The well-formed check already has checks for normal form, so I think it is not needed to add checks at other sites. https://github.com/tlc-pack/relax/blob/83adb870240a13ae75d0dcfac93e81bb2f3bcf59/src/relax/analysis/well_formed.cc#L42-L56 https://github.com/tlc-pack/relax/blob/83adb870240a13ae75d0dcfac93e81bb2f3bcf59/src/relax/analysis/well_formed.cc#L244-L249

slyubomirsky commented 1 year ago

Yeah, I think the IsLeafNode helper function would take care of that. You can put in an xfail test case for the well-formedness check to make sure it would be rejected.

yongwww commented 1 year ago

thanks @MasterJH5574 @slyubomirsky @tqchen for the review!

tqchen commented 1 year ago

thanks @yongwww please also send a PR to patch apache/unity

yongwww commented 1 year ago

thanks @yongwww please also send a PR to patch apache/unity

Thanks @tqchen !, I will do that. Just took a look at the tvm/unity branch, well_formed pass is not there yet. I guess it would be good to wait until the pass is landed.

tqchen commented 1 year ago

well-form pass now lands, please send the patch in :)

yongwww commented 1 year ago

well-form pass now lands, please send the patch in :)

thanks, sent it out in tvm/unity .