[Mlir-commits] [mlir] [mlir][Transforms] Dialect conversion: Skip materializations when running without converter (PR #101318)

Fangrui Song llvmlistbot at llvm.org
Thu Aug 1 09:31:04 PDT 2024


MaskRay wrote:

> @MaskRay : the PR is described as:
> 
> > Experiment only. Do not merge.
> > TODO: test case
> 
> I don't get why it would be OK to merge here, especially without test case.
> 
> I also don't understand the urgency: unless there is an **upstream** bot broken, it's doesn't seem like an OK thing to merge this! I don't see a valid justification in the comment you're referring to either.
> 
> Downstream specific issues should be handled downstream (you can apply this patch in your fork downstream, you can revert the original patch, you can disable your failing tests, you can wait a few days for this patch to merge upstream, etc.), there is no reason to mess-up with upstream.
> 
> It wasn't even passing the code formatting!

You are right, it was not OK to merge here. Apologies, the situation wasn't clear to me and as I do not normally contribute to MLIR, I made a bad judgement here. I did perform some downstream tests and verified that the workaround (101148) would work. More thorough investigation was necessary before reverting or merging and I should have exercised greater caution.  Sorry for jumping the gun.

https://github.com/llvm/llvm-project/pull/101318


More information about the Mlir-commits mailing list