Revert "[Unity][Parser] Check well-formedness in the parser"#16769
Revert "[Unity][Parser] Check well-formedness in the parser"#16769
Conversation
This reverts commit 6c701fe.
|
Could we perhaps just revert the driver changes if that's what's causing the MLC failure? They are unrelated to the main functionality of the PR and I would rather have the checking for future tests |
There was a problem hiding this comment.
@tqchen Can this PR also add a test case for the behavior that should be preserved? From your comment here, MLC is currently relying on untested functionality in TVM, and I don't think we should revert based solely on untested functionality. Doing so would mean that there would neither be a path forward for re-applying the breaking change, nor any protection for the same breakage occurring in other PRs
I think it makes sense to revert commits that break important workflows, so long as there's a reproducible example of the failure. Otherwise, the responsibility for reproducing that failure mode falls on a developer who wasn't actually seeing the failure.
Edit: Not requesting a perfect unit test that exercises the specific bug, just a high-level test that would trigger the bug in CI.
|
@slyubomirsky reverting the driver change also works, just wonder if that will break some of the existing tests |
|
The test should have already been broken, oddly! The error was not triggered by anything I added. |
|
agree that we should have a carved out testcases here, will followup in comment soon, as well as in the original thread I believe it was along the original lines of comment in the changed order, which i assume we say that we cannot merge dyn smem beyond the kernel boundary, // MergeSharedMemoryAllocations must be applied after SplitHostDevice
// because the merged allocation site is at the beginning of each device function
mixed_pass_list.push_back(tir::transform::MergeSharedMemoryAllocations()); |
|
|
|
OK, given that case we should perhaps first revert the driver order only and disable(then look into the failed tests). We will still get a UT for the ordering case, which I assume would might relates to getting that dlight function compiled, and then reduce to a smaller case |
| int dev_type = target->GetTargetDeviceType(); | ||
| if (!(dev_type == kDLCUDA || dev_type == kDLMetal || dev_type == kDLROCM || | ||
| dev_type == kDLWebGPU)) { | ||
| if (dev_type != kDLCUDA) { |
There was a problem hiding this comment.
Yes, this was causing a test failure. I will find the link to the discussion
| if (op->thread_binding.defined()) { | ||
| auto old_thread_binding = op->thread_binding.value(); | ||
| auto* ptr = old_thread_binding.CopyOnWrite(); | ||
| ptr->var = old_thread_binding->var.copy_with_dtype(new_loop_var.dtype()); | ||
| n->thread_binding = std::move(Optional<IterVar>(std::move(old_thread_binding))); | ||
| } |
|
#16770 Smaller-scale reversion |
|
Thank you, @tqchen , for the follow-up, and for helping to make the more targeted reversion! |
|
closing in favor of #16770. Thanks @slyubomirsky @Lunderberg |
3 participants
Reverts #16569