Skip to content

Conversation

@Lunderberg
Copy link
Contributor

Prior to this commit, the FuseTIR pass had custom logic to determine whether a PrimFunc should be kept in the output IRModule. This commit replaces this check in FuseTIR with a post-processing by DeadCodeElimination.

@Lunderberg
Copy link
Contributor Author

@slyubomirsky Would this resolve the problem encountered in #16462 ? I think this would avoid throwing out the in-place updates, and would also allow handling of context-dependent fusions, depending on whether the call site could support an in-place operation.

Prior to this commit, the `FuseTIR` pass had custom logic to determine
whether a `PrimFunc` should be kept in the output `IRModule`.  This
commit replaces this check in `FuseTIR` with a post-processing by
`DeadCodeElimination`.
@Lunderberg Lunderberg force-pushed the transform_relax_fuse_tir_postproc branch from 33c3d6a to de8b8b6 Compare January 26, 2024 00:13
@slyubomirsky
Copy link
Contributor

slyubomirsky commented Jan 26, 2024

Could you add the test case from that PR? It probably will solve that problem. No longer necessary.

return call;
}

tir::PrimFunc fused_tir = FusedTIRConstructor::GetFusedTIR(orig_mod_, old_gvar);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure we aren't likely to call this more than once per global var? As long as it's not likely or costly, this approach is probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably would be called several times. The fused Relax function is generated once per pattern match in FuseOpsByPattern, and is then de-duplicated, leaving many calls to a single kPrimitive function. This would generate the fused TIR function once per callsite, after which the resulting functions would be de-duplicated. Whether that is good or bad would probably depend on the usefulness for call_tir_inplace, so I'll take a look at #16487.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh so there's probably no issue with calling it more often than in the current way the pass is written. That seems good to me.

@slyubomirsky
Copy link
Contributor

See 16487, the in-place case will be handled.

@Lunderberg
Copy link
Contributor Author

See #16487, the in-place case will be handled.

Sounds good. I think I still like having this change overall, as it avoids having duplicate logic for DCE. When 16487, I'll rebase this PR on top of it.

@Lunderberg
Copy link
Contributor Author

Closing this PR, as it is incompatible with #16487. A similar bugfix is applied in #16565.

@Lunderberg Lunderberg closed this Feb 13, 2024
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