In D144782#4172299, @mehdi_amini wrote:

Your commit above is just not warranted IMO as it does not fix any upstream problem.

Submitted https://github.com/llvm/llvm-project/commit/f2cdccc034d0696f186f55d6826dd6ac40e4d3d5 for now that restores the preloading behavior, and fixes the test that was failing in our internal configuration. If we really need to support running commands that expect to be given an EOF input to work, we can work on our internal lit runner to support that, but I would prefer to have a heads-up for that before.

Yes, "registered" is what I meant.

Right, any convenient ways to just get a list of loaded dialects from a script. mlir-opt --show-dialects --version doesn't work (I guess version is processed before we get to --show-dialects), and echo "" | mlir-opt --show-dialects is a weird thing to have.

Ah, the --show-dialects option is no longer a stand-alone thing, but an additional printout, where you could also do other things. So I guess in my example it's still waiting for some IR as input.

This seems to cause mlir-opt to hang when passed the --show-dialects option. Ran it through bazel:

From my experience integrating MLIR into Google's internal repository, breakages from changes like these seem to be very frequent (e.g., the two already mentioned above), so I don't see why this change should be blocked while others get a pass. If we want to be more careful about API breakages, then we should first formalize this somehow and apply the same rules to everyone.
I never considered the printed version of MLIR stable, and I'm not sure that its stability should be the goal that prevents improvements to the printing format.
Regarding churn, we should not forget that not improving the printed format also causes churn every time someone is reading or modifying it.
I think that redundancy in IR causes more issues than it solves, so I would prefer landing this change.

Also lower math.tan for ROCDL

In D137786#4032742, @arsenm wrote:

I don't think we even have a legalization implementation to expand these. You almost certainly intended to use llvm.minnum/maxnum

In D137655#3919674, @arsenm wrote:

In D137655#3918964, @gflegar wrote:

In D137655#3917359, @nikic wrote:

I don't think this is right either. LLVM defines minnum according to the old semantics, which don't specify an order between zeroes. We'd need a separate ISD opcode for minnum according to 2018 semantics.

Interestingly though, for the NVPTX backend this will end up producing the correct code, since minnum is lowered to the min/max PTX instructions, which defines the 2018 semantics. (I do agree though that the intermediate code is not correct.)

It's not OK to have wrong intermediate code. We do have the "new" semantic opcodes already in FMINIMUM/FMAXIMUM

It's even less OK to fail altogether (which is what is happening without this patch). And we're not talking about the new semantics for FMINIMUM/FMAXIMUM, but for the new semantics of FMINNUM/FMAXNUM (the +/-0 handling changed).

Formatting fixes

I think this is the best we can do short of adding a new op. We only do the expansion for the NVPTX backed, and don't support it otherwise. In the NVPTX backed, the intermediate code still ends up being semantically incorrect for +/-0.0, but since FMINNUM/FMAXNUM lower to PTX min/max, which do implement the 2018 semantics of those ops, the final PTX ends up being correct.

Limit expansion only to NVPTX backend

In D137655#3917359, @nikic wrote:

I don't think this is right either. LLVM defines minnum according to the old semantics, which don't specify an order between zeroes. We'd need a separate ISD opcode for minnum according to 2018 semantics.

In D137655#3917621, @nikic wrote:

In D137655#3917468, @gflegar wrote:

In D137655#3917359, @nikic wrote:

I don't think this is right either. LLVM defines minnum according to the old semantics, which don't specify an order between zeroes. We'd need a separate ISD opcode for minnum according to 2018 semantics.

Unfortunately, I don't have the bandwidth to chase this rabbit hole further, especially since our use case is insensitive to what happens for -0 and +0. I can add a TODO comment to fix this. Though I would still argue for submitting this, as it is correct modulo -0/+0, which is far preferable to the current state where we have a silent failure (and produce invalid code) for the backends that attempt to expand the op (like in NVPTX).

That sounds like an unrelated bug. If an operation is Expand, but we don't support expanding it, shouldn't that result in an isel failure?

In D137655#3917359, @nikic wrote:

I don't think this is right either. LLVM defines minnum according to the old semantics, which don't specify an order between zeroes. We'd need a separate ISD opcode for minnum according to 2018 semantics.

@nikic 0 handling should be fixed now

Change lowering to minnum/maxnum + NaN check

In D137655#3916933, @nikic wrote:

In D137655#3916906, @gflegar wrote:

In D137655#3915463, @nikic wrote:

Doesn't this handle signed zero incorrectly?

I believe it is:

For FMINIMUM:

Tmp1 = -0.0, Tmp2 = +0.0  =>  Tmp1 ULT Tmp2 == True   =>  Tmp3 = Tmp1 = -0.0;     Tmp2 UO Tmp2 = False  =>  Result = Tmp3 = -0.0

Isn't -0.0 ULT 0.0 false, because negative zero and positive zero are equal?

In D137655#3915463, @nikic wrote:

Doesn't this handle signed zero incorrectly?

We can achieve what we need by adding LoopInvariantCodeMotionPass. Abandoning this.

In essence, the issue is that tiling, mapping to gpu and the transformation to gpu dialect have to run after each other without being disturbed by other passes.

Worked like a charm.

Prevent merging loops with attrs instead

Sure, I can change to addWithLabel instead.

Minor formatting

Make combine a regular function, use CHECK-DAG, formatting