Could you plz rebase on master? It does not seem to apply anymore. And fix clang-format while you are there.

Please note that we tend to prefix the header of the commit message with [mlir] so that people on llvm-commits@ can understand the changes are restricted to MLIR (many more people might have a say if actual LLVM IR intrinsics were broken)

Please update the commit message from "fixed tanh lowering" to "add tanh lowering". It was not broken (which would mandate a fix), it was never implemented to start with. Good to go otherwise.

I suppose that, in general, there is no strict correspondance between the number of type suffixes in an overloaded intrinsic and the number of operands. So having one type may make sense for fmuladd, but may not make sense for some platform-specific vector select-like intrinsic with three arguments that would require two types. I would suggest renaming BinaryIntrinsicOp to BinarySameArgsIntrinsicOp if you do this change. Or even dropping it entirely and keeping an explicit definition for copysign and fmuladd since I don't see any other users of the Binary and TernaryIntrinsicOp classes.

Thanks! I have a couple of comments.

Looks generally okay to me. I don't see the point of cloning before erasing, but can live with it pending a clean-up.

Anything else @nicolasvasilache ?

In D73362#1839215, @antiagainst wrote:

Thanks Alex for adding this! Have you considered adding an additional parameter (defaulting to C++ enumerant symbol) for the string representation instead of duplicating all the cases?

Thanks, I like the simplification.

The change looks innocuous, I would have landed on approval, too.

Thanks!

I'm supportive of this change as long as Mehdi's and River's concerns are addressed.

Looks good in general, I am suggesting to push it one step further.

I reverted this change since it broke dependent projects, and the modeling generally looks incorrect.

Minor comments only.

@tra, your rule adds you as blocking on all of MLIR :)

The change is mostly renaming/clean-up. I only have minor comments, feel free to land after addressing.

To clarify further, LLVM dialect in MLIR is not a new IR, it's essentially a representation of LLVM IR using MLIR. It's supposed to minimize the cost of translation from MLIR to LLVM IR. While it does diverge from LLVM IR proper, it does so for modeling reasons: there's no first-class constants in MLIR, neither are there phi nodes. Changing the name of an enumerand locally to the LLVM dialect will increase the cost of translation, which goes against the goal of this dialect. That being said, I do agree that we should use this opportunity to reflect on LLVM IR design and update it following the proper process.

Changing LLVM IR should _not_ be in this commit, it's semantically different and requires separate discussion with relevant people involved. I also don't see a compelling reason to block this from landing until LLVM IR changes (or decides not to). LLVM dialect models LLVM IR in its current state, whatever the state is.

address review

I would like to make it a bit more useful (e.g. let the caller insert custom builders/printers/parsers) before plugging it into the flow directly. So it will come in several follow-ups. I'll make the requested changes directly in the commit.

Added in a922e23101b882e22fc14ffb78b5857954dc86f3

https://llvm.org/docs/DeveloperPolicy.html#code-reviews point 3

I'm not sure I understand what happens to the Op operands that are supposed to be tensors in individual linalg_ constructors. The comment says outputs should not contain tensors, but in individual constructors there's no generic outputs, and some StructuredIndexed *must* be passed in.

I think you can push fixes for upstream breakages, especially in the part you own, without pre-review.

Could you please elaborate what exactly was causing problems? The original diff is https://reviews.llvm.org/D72336.

@mehdi_amini @nicolasvasilache @herhut Let's take some time and discuss builder APIs outside this diff (also involving @rriddle). My basic observations are that (1) writing structured IR, as in "with nested regions", looks unnecessarily complicated with builders, arguments are the same as those against goto-style programming; (2) a lot of IR construction internally happens in rewrite patterns, where location almost always remains the same, that of the matched operation root; (3) current EDSC APIs are contentious partly because it is unclear when reading the code when the function call creates the IR vs. when it's just a function call.

Thanks! Most of my comments are minor.

I think this goes into the right direction. The issue with this patch is that it removes some libraries from add_dependency for some targets, but not all of them, thus creating more inconsistency than we already have. Could you make it consistent for all targets and, if possible and in a follow up, add a doc that says how to properly set up a library for a new dialect.

The website uses a different layout anyway so many links won't work on GitHub, we don't expect them to. We prefer to be consistent and have the website working correctly.

What is the status re EDSC discussion?
Were the pointers I sent offline enough to give a good picture / do you see how to followup on this to use (and maybe extend) EDSCs?

Something went wrong with formatting in the commit description (mixed double space prefix and backticks?). Please reformat and feel free to land.

It's in the website repo https://github.com/llvm/mlir-www/tree/master/website/content/getting_started and visible on mlir.llvm.org. Can you only fix the broken reference?