Pass container instead of begin/end to sort

In D139843#3988794, @jpienaar wrote:

And this is a more fine grained version of mlir-print-ir-after ? What is missing there? Triggering only particular pipeline, particular part of pipeline?

NFC: cleanup some ugliness

I often add a simple dump command inside of a pass at the beginning or end when I'm quickly trying to gather some data and don't want to dump everything. I considered making a tiny rfc regarding this to see if others prefer another way, but want to at least first run it by a couple of folks. I'm sure there's several ways to do the same thing, but this would be my preferred way of debugging a pipeline not presented in a test using opt and before my morning coffee.

I'm leaving a few high level comments first. I think this abortOnFailedAssert=false case should be handled by a separate test pass instead.

It looks like you will also need to run clang-format on this. I use git clang-format --style=LLVM HEAD to format my patches before landing (then you can use arc diff to upload the final patch this time and I can submit it on your behalf).

In D136727#3898124, @rriddle wrote:

In D136727#3898123, @mehdi_amini wrote:

Do you think we could wait until after the LLVM Dev meeting MLIR Summit to land this? We're kind of scrambling around these days and still working on preparing for this to land.

Yeah, that's fine.

In D136727#3886133, @rriddle wrote:

In D136727#3884816, @tpopp wrote:

Are you still going to wait until early November, or are you going to commit it this week?

I'll wait until early November (like in the post), just sent this out to prepare for that.

Are you still going to wait until early November, or are you going to commit it this week?

In D126305#3871463, @jfurtek wrote:

It seems weird to add a dependency on the Arith Dialect here.

This is a good point - let me see if I can avoid that...

Awesome! You use copyBeforeWrite and skipAnalysis. I think it would be better to use the same name consistently, like the other options/flags, unless skip-analysis is expected to grow to modifying multiple option values(?)

Drive by warning. If this patch is depending on a non-landed commit, this comment might be wrong. It looks like one python binding test case will fail with this patch:

Unfortunately, it looks like the version of clang on the pre-merge bazel build bot is too old, or similar, so I am abandoning this attempted workaround.

Move fuse-ld to ci

Move flag from generic_clang to ci until pre-merge is fixed

Remove overlooked line.

linkopt can't be overriden with empty strings, so workaround differently.

Fix flipped names

Remove definition of method after only declaration was previously removed.

Nice!

Drive by nit... The important parts to me in regards to this change are:

1. The change to the assembly format
2. The change in types that break C++ code interacting with this class

I'm resigning as reviewer as I just wanted to voice my concerns regarding populate methods creating global effects. I wish more tests used the canonicalizer just to ease the mess that some FileCheck tests are, and I think targeted canonicalizations can make sense.

I did add the canonicalization patterns to a downstream and have observed that they do "help" a lot of real cases I was seeing get to a fully decomposed state.

This creates a situation where unrelated linalg::generic ops can be canonicalized as a side effect. This has happened multiple times in linalg infrastructure and is always confusing to a user when the populated patterns are actually doing much more than they claim to do because of these global canonicalizations. I can understand the benefits of this when only applied to the newly created ops, but as is this creates confusion.

Respond to style fixes and improve the unrealized_conversion_cast conditional.

I'm going to land this as I suspect there are no strong feelings over this change and there was a lack of failing tests to show people might rely on this, but if anyone has concerns, please feel free to revert and raise your concerns, as this commit was not in review for long. For any abstract concerns though, please keep in mind that the existing code failed in multiple situations, and only worked in limited code paths.

Sorry to delay on this for so long. I am proposing, instead, a TensorToLinalgPass here: https://reviews.llvm.org/D125384

In D124649#3486240, @lattner wrote:

Hello, why were parseOptionalAssignmentListWithTypesand parseAssignmentListWithTypes removed?

This was simply dead code in the tree. I'm sorry I didn't realize other people were using it, I didn't mean to break anyone intentionally. There is a simple replacement with parseArgument and parse comma separated list, though we could add back if it would help with your internal build and integration for awhile.

-Chris

Hello, why were parseOptionalAssignmentListWithTypesand parseAssignmentListWithTypes removed? This seems to be a common pattern in MLIR where functionality is provided but untested and unused inside of core and then freely removed while breaking users. If this is the intended progression of the API, a deprecation would be appreciated, so people can raise concerns that used functionality is being deleted. If this being freely removed because users of the code outside of the LLVM repository don't impact this decision, it really indicates that MLIR core needs to start providing some form of a stable API that people can rely on rather than the current (small) chaos caused by many changes.

Landing now instead as it is low risk, and I mainly wanted to confirm with River.

I'll leave this until Monday to submit in case the comment in code that insert is soft deprecated is out of date.

Update test cases

Clang documentation for ASAN, as I feel like everyone has different flows for using it: https://clang.llvm.org/docs/AddressSanitizer.html

I am reverting this change as it has a stack-use-after-scope error. I suspect this is due to the call to expandAffineMap returning a SmallVector which is then referenced with the ValueRange indices inside of the if block and then used outside.

Hi, I added a fix forward for this change, as a rollback was hard due to another change, but it would be ideal to confirm that it is okay, as I'm not sure exactly what you were going for: https://github.com/llvm/llvm-project/commit/64b918852c09db6461d4c4c97df288c7b7fee865

In D114358#3149334, @rriddle wrote:

Was the patch updated?

Store empty attributes instead of nullptrs.

In D87202#3103520, @jamieschmeiser wrote:

Hi @tpopp, can you please explain to me the nature of these failures? The code changed here is only active under options that are not being used in those tests so I expect that the errors are unrelated. I have not received any notifications from the build-bots of a problem so far. I am also unclear as to why there are build issues on those other platforms. Are you asking me to revert this?

Hi Jamie, this code has test failures: https://reviews.llvm.org/B131761

Drive by comment

Move the unsupported logic to the caller.

I've been somewhat convinced that this could lead to subtle bugs in the future, so we should leave it to the callers to handle this issue each time they want to build this op during dialect conversion, so I'm going to abandon this.

Move assertions to NamedAttrList::push_back

Move the assertion logic to NamedAttrList.