Looks great.

In D95394#2521877, @stephenneuendorffer wrote:

This seems to me like a 'tip of the iceberg' kind of solution. What happens if the attribute needs to be transformed? What happens if there is not a 1:1 correspondance between input operations and output operations in a transformation? etc. etc. I'm not sure what the right solution is, but this seems to be a narrowly scoped infrastructure solution to a tensorflow problem. I'd be much happier if this solved other obvious problems.

This looks really great! A bunch of minor comments to address.

Can you please add a proper commit summary for the rationale (basically whatever is in the discussion now)? Also, as @jpienaar points out, the commit title isn't really accurate.

Missing a test case?

Thanks very much for fixing this!

@ThomasRaoux, not on a directly related note, but this file VectorOps.cpp is taking nearly 12-15s to build on a fast workstation -- directly increasing build times / critical path towards mlir-opt and other targets for everyone. It'd be great to consider refactoring among next steps.

Mostly minor comments.

Thanks for this significant update! Some superficial comments to start with. It may be good to handle the pass documentation update as well, starting with an update to the summary. It's currently emptly: https://mlir.llvm.org/docs/Passes/#-affine-loop-fusion-fuse-affine-loop-nests
You could mention about the fusion strategies producer/consumer and sibling ones as well. Here's the previous doc paragraph that disappeared (was overwritten) when pass documentation was migrated to auto-generated stuff.

Looks great!

In D92991#2444781, @nicolasvasilache wrote:

Thanks!

I'm -1 on the proposed name change: LLVM has {insert|extract}{element|value}, op names are globally consistent with that terminology.

Nothing major, but I tend to prefer tensor.get_element to tensor.extract for two reasons: (1) extract in standard data structures/ADT literature also involves removing the element out of the structure (eg. extract_min/extract_max for heaps, priority queues), (2) extract is more suitable for containers but this is an immutable entity.

This looks great! Thanks.

Nice. Looks like you missed adding test cases.

It's been exactly two months since I submitted a review!! - this was I think a simple patch. I've forgotten all context! @dcaballe - could you please review this as it's directly on top of your work? @flaub please go with @dcaballe's review.

I think the builtin dialect isn't documented anywhere:
https://mlir.llvm.org/docs/Dialects/
(Crazy question: do we really need to even call it a dialect? Can't it just be BuiltinOps.h?)

In D90899#2391979, @silvas wrote:

In D90899#2390356, @bondhugula wrote:

Is "call graph types" the best term here? This doesn't have to do with call graphs but with function signatures.(?) The older naming along the lines of FunctionSignatureConversion or DecomposeFuncSignature or UnpackFuncSignature looks clearer than anything with "CallGraph".

I'm using the term "call graph types" as a stand-in for "function signatures, call signatures, and return signatures". That is, types that flow along call graph edges.

I was also keeping an eye towards eventually generalizing this beyond std.func/call/return. In that more general setting, I think "call graph" is much clearer, because different dialects might have objects in their call graphs that are not called "function". For example, the concept of a first class callable object has many different names: lambdas, closures, delegates. Using the name "function" doesn't sit well with me in that more general setting. In all cases though, the set of patterns we need are the same: one on CallOpInterface, one on CallableOpInterface, and one on something that represents returning control flow from a CallableOpInterface. Once we have extended those interfaces appropriately, the patterns in this pass would be updated to MatchAnyOpTypeTag patterns written over those interfaces and everything would just work.

I guess it depends on how one looks at the pass: is the pass fundamentally updating function signatures / callable signatures (and everything else is just changing to match), or is the pass to be viewed as decomposing types along call graph edges. I was thinking about the pass as the latter. What do you think?

LGTM - thanks! Just some minor comments (please see if the "mixed" test case could be improved for coverage).

Would a test case be applicable here? I assume this leads to a pattern match failure and the op staying as is in a simple case.

Is "call graph types" the best term here? This doesn't have to do with call graphs but with function signatures.(?) The older naming along the lines of FunctionSignatureConversion or DecomposeFuncSignature or UnpackFuncSignature looks clearer than anything with "CallGraph".

This appears nice to me. Some superficial comments to start with.

This looks like a really great cleanup! Thanks! A list of minor comments.

Looks good. Please address the remaining minor comments.

But this would break the mhlo to lmhlo conversion where the canonical form of an incoming TF model lowered to mhlo is to return a tuple of tensors, and that tuple gets unpacked into a list of memref output arguments on the function. If this functionality can be improved with a better separation of concerns, it may be good to do that. But we shouldn't delete this until then! The way it is now: the functionality is clearly defined with its own proper test cases. And it has an out of tree user in the mlir-hlo repo (-hlo-legalize-to-lhlo pass) - please don't remove this! :-)

Do you want to add a to in the syntax?

Mostly minor comments on readability.

In D88341#2297260, @ThomasRaoux wrote:

In D88341#2296114, @mehdi_amini wrote:

->

func @distribute_vector_add(%A: vector<32xf32>, %B: vector<32xf32>) -> vector<32xf32> {

%C = simt.execute %A to %Aelt : vector<1xf32> affine_map<()[s0] -> (s0)> 
                                 %B to %Belt : vector<1xf32> affine_map<()[s0] -> (s0)> {
  %0 = addf %A, %B : vector<1xf32>
  yield %0
}

Thanks for adding this support. Some minor comments. I'll be able to review post the refactoring.