NoSideEffect does not seem to fit to me: there is an effect for allocation. It should also write to its output.

LGTM

LGTM!

Are the changes in mlir/lib/Conversion/AsyncToLLVM/AsyncToLLVM.cpp bug fixes? Can you add IR tests for this (and commit separately so there is a proper commit description)?

LGTM

In D90554#2408163, @spatel wrote:

So to clarify policy, we reverted patches when (1) a test based on an experimental intrinsic was crashing and (2) that crash is easily reproducible independent of this patch as shown here:
7ae346434

I understand that we revert first and ask questions later, but should that be the rule for experimental code?

@stellaraccident / @ftynse can you double check on the deleteUserData part and the shared_ptr?

Can you expand a bit on the use-case for deleteUserData? When is the user not in control of the lifetime? This seems all kind of tied to the context in the end?

(Drive by comments)

Oh I didn't read about https://reviews.llvm.org/D87326 before, isn't the work there making obsolete all of the assertions you (and me before) just added by making this behavior safe?

Can you make more clear in the description the motivation for this patch? Is this mostly about iterator invalidation? Is this expected to make code safer?
I'm not sure about making the mental model a bit harder by having different iterator invalidation behavior implicitly based on T, WDYT?

[mlir] Require std.alloc() ops to have canonical layout during LLVM lowering.

Sweet!

I tweaked slightly the doc and the commit message:
We like to tag commit messages with "NFC" when they don't modify an existing behavior and don't include tests (this change is a bit borderline, it *could* be plumbed and tested with a pass)

Seems like a "hack" to me, why not fix the source?

In D91470#2395621, @vitalybuka wrote:

Some bots are still broken
http://lab.llvm.org:8011/#/builders/85/builds/811
http://lab.llvm.org:8011/#/builders/74/builds/857
http://lab.llvm.org:8011/#/builders/99/builds/670

In D91470#2395433, @mehdi_amini wrote:

We should dig in the OpenMP side as well.

What do you mean by that?

Anyway, for now, the need for ensuring folds result in the same Type is a reality and that is why ElementwiseMappable is defined that way.

In D91470#2395424, @clementval wrote:

Yeah it makes sense to have tests directly in llvm. Sorry for that I blindly followed the OpenMP stuffs and I should have think twice about it.

In D91396#2394526, @silvas wrote:

In general I agree that ops that accept tensor<?xf32> should probably also accept tensor<2xf32>. (though probably not calls/functions, since those check Type by pointer equality, and I think we want to keep that?)

(Feel free to just push directly this kind of fixes)

I think it is very unusual to have a library in LLVM that isn't used in LLVM and so does not come with any testing, so yes it'd be better to do so.
I'm not actually sure if llvm-config just relies implicitly on this?

In D91470#2395391, @clementval wrote:

You are right, at the moment it is not connected directly to LLVM itself. It was done like this a little bit blindly follow what was done for for the libLLVMFrontendOpenMP.

Won't we end up with a different behavior (with respect to iterator invalidation) based on the platform?

It seems like it is following what was done for OpenMP, however OpenMP is exposing LLVM IR Builder which connects it directly to LLVM. The directive part of the OpenMP library isn't tested either (uhhhh) in LLVM, it just does not expose the issue because it is bundles in the same library as the IR builders which are tested.

@clementval : it isn't clear why this is in llvm/lib/Frontend/? Should this be a top-level frontend/lib kind of structure? There isn't any user of this inside LLVM right now other than flang and it isn't clear to me how it connects to LLVM itself.

I send out https://reviews.llvm.org/D91470 as another take on this.

I know this fixes a bug, but I don't think this is quite correct.

Fix one clang instance failing this assertion

Update with Sean's suggestion

It fails for me as well: configuring just LLVM alone and running ninja check-llvm instead of building first everything with ninja alone.

+1 on both points!

In D91396#2392879, @silvas wrote:

In D91396#2392877, @mehdi_amini wrote:

In D91396#2392850, @silvas wrote:

From the rationale:

/// - 3. guarantees that folding can be done across scalars/vectors/tensors
///   with the same pattern, as otherwise lots of special handling of type
///   mismatches would be needed.

I don't quite get what is the issue with this ?->2 ? Can you elaborate a bit? Do you have an example of a pattern maybe?

Consider xor(xor(a, b), b) -> a

%0 = xor %lhs, %rhs : (tensor<?xindex>, tensor<2xindex>) -> tensor<2xindex>
%1 = xor %0, %rhs : (tensor<2xindex>, tensor<2xindex>) -> tensor<2xindex>
"only_accepts_2xindex"(%1) : (tensor<2xindex>) -> ()

only_accepts_2xindex might be defined so that it only accepts tensor<2xindex>. If you apply the fold, you will replace %1 with %lhs, which will create invalid IR.

In D91396#2392850, @silvas wrote:

From the rationale:

/// - 3. guarantees that folding can be done across scalars/vectors/tensors
///   with the same pattern, as otherwise lots of special handling of type
///   mismatches would be needed.

SameOperandsAndResultShape allows `"foo"(%0) : tensor<2xf32> -> tensor<?xf32> but ElementwiseMappable does not.

LGTM

In D90884#2392543, @dexonsmith wrote:

having llvm::Vector be bigger than std::vector seems dangerous to me, simply because it would be surprising.

You may need a refactor like Alex did here: https://reviews.llvm.org/D85650 ; what about a factory method returning ErrorOr<Triple> ?

Can you add a test for this?

LGTM

Do you have tests for all this?

I'd prefer to avoid SmallVec: it is too close to SmallVector (it is even a prefix, which will conflict with autocompletion in IDEs).
llvm::SVec or llvm::SVector on the other hand seems different enough to me to avoid confusion, and the using definition you propose seems like a good tradeoff to me!

DefaultSmallVector is quite a "mouthful" though, I'd have thought we'd look for a "default container" that is easy to type if we have it everywhere.

Reverted in f917356f9ce0 ; I suspect a static constexpr in a class missing a definition out of class (required pre-c++17).