Address comments

Reduce redundant code

You are absolutely correct, did not find that one.

Move location of call before analysis

If it’s possible to not revert that would be preferable as @jdoefert @vchuravy I and others have many applications that would have significant slow down if reverted.

I'm personally of the opinion that building a special case for an illegal use of the invariant metadata (e.g. where it would guarantee the memory is not changed), would produce more fragile code -- since it'll be harder for someone who accidentally creates such illegal metadata to find the actual source of the bug.

Fix style

Fix test

Actually run linter

Run linter

Address feedback

Since you say it's possible to get the wrong cost, is it possible to create a test case that highlights this?

Since I feel like this recent conversation is distinct from the PR, I think it deserves its own issue for visibility and tracking.

In D122531#3454495, @mehdi_amini wrote:

You likely didn’t rebase for a while and the CI may not do it itself

In D122531#3454481, @NathanielMcVicar wrote:

I posted this on the commit, sorry! Meant to do it here.
This appears to have broken a number of tests on the Windows buildbot: https://lab.llvm.org/buildbot/#/builders/13/builds/19731

LLVM ERROR: TypeID::get<`anonymous-namespace'::ClonePass>(): Using TypeID on a class with an anonymous namespace requires an explicit TypeID definition. The implicit fallback uses string name, which does not guarantee uniqueness in anonymous contexts. Define an explicit TypeID instantiation for this type using `MLIR_DECLARE_EXPLICIT_TYPE_ID`/`MLIR_DEFINE_EXPLICIT_TYPE_ID` or `DEFINE_EXPLICIT_PRIVATE_INLINE_TYPE_ID`.

Uh, this transitive mapping seems wrong to me, isn't it? If it is: should we fix this instead?

In D122531#3453157, @mehdi_amini wrote:

I didn't quite follow the description of the unroll logic, but can you add a test? It may be easier to figure it out then.

Fix linking

Fix format

Add simple clone test

In D122531#3410912, @ftynse wrote:

This needs a standalone test that must not depend on any affine stuff. It can be implemented as a simple pass defined in test/lib/IR, there's already a bunch of such passes.

However, I am not convinced the standalone test can be constructed. Looking at the semantics of the change, it moves the addition of the operation results to mapper to be performed after cloning the regions of this operation rather than before. In valid IR, SSACFG regions of an operation cannot use its results, so the presence of these results in the mapper should not have any effect on the contents of the cloned regions. Maybe the IR that Operation::clone is being called on is already invalid. Maybe a nastier thing happens: Value is essentially a pointer, so if the operation defining it is erased without also updating the mapper and another operation is created, the pointer may get reused to point to an irrelevant value. In any case, I suspect it's the unroll pass that needs a fix.

Remove unnecessary change

Polygeist is a (hopefully soon to be LLVM incubator project: https://discourse.llvm.org/t/rfc-polygeist-llvm-incubator-proposal/60890), that, among other things, is a Clang-based C/C++ frontend for MLIR. As such, we kind of expect to be built alongside clang and borrow the header. As the project gets closer to upstream, hopefully the include sharing will actually be more explicit (and will be made an upstream PR regardless after some other technical debt and paper deadlines).

What precisely is the regression and the origin of it? This patch essentially aims to restore the information which was lost during a prior Phase Ordering change in https://reviews.llvm.org/D99249 which caused a large number of performance regressions. What is the behavior of your code prior to https://reviews.llvm.org/D99249 landing?

Address comments and fix formatting

Address comments and add test without else

Fix build

Fix format

Fix clang test

Add test file

Remove scope from name

Change name/docs