In D125440#3519346, @DavidSpickett wrote:

Fixed in 9e469ced42cd9335baffd906bed17fd81ce4e6e8, all good now.

Could you add a test somewhere?

In D125440#3518288, @DavidSpickett wrote:

Turns out we specifically don't include tablegen in the llvm shared library https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-shlib/CMakeLists.txt#L20.

If I do it will link but you get:

FAILED: tools/clang/include/clang/Sema/AttrParsedAttrList.inc
<...>
: CommandLine Error: Option 'o' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

Given that this is a language server would it make sense to disable building it if LLVM_LINK_LLVM_DYLIB is on?

I'm fine with landing this to unbreak things, but we really shouldn't need to define this IMO (the infra should automatically handle it).

In D125651#3514675, @jpienaar wrote:

Did none of the cmake files need to change? If I'm reading this change correctly it just bails out if no dialect is specified, and so I'd expect some to fail (well except if the default cmake config already does set this flag)

In D125651#3514675, @jpienaar wrote:

Thanks, and yes with Marius 's change for doc includes (couple months back :-)) there isn't really reason to have multiple be generated website side. Being explicit here is better.

Did none of the cmake files need to change? If I'm reading this change correctly it just bails out if no dialect is specified, and so I'd expect some to fail (well except if the default cmake config already does set this flag)

In D125051#3516215, @jpienaar wrote:

In D125051#3495776, @rriddle wrote:

In D125051#3495775, @mehdi_amini wrote:

they can get the same behavior by using the SameOperandsAndResultShape and SameOperandsAndResultElementType traits.

Should SameOperandsAndResultShape be renamed CompatibleOperandsAndResultShape?

Yeah, let me do that in a followup.

But the user really wants them to be the same dynamically. E.g., if you have ?x? and ?x?, the same shape is where the four unknowns are dynamically pairwise equal. Treating ?==? as true, isn't the goal as these could be different dynamically, but statically they aren't known. And restricting shape equality to static interpretation is more lax as it doesn't require dynamic equality but merely about static type. Yes verify wise equivalent'ish, but concept captured differs. Same here represents an equality constraint, statically we don't have sufficient information to reject so we have to be permissive. But it means where you have ?x? and 7x?, the first shape's unknown can be inferred to be 7 and propagated. That's not true for compatible.

Looks good. I think we can do some more generalizations of the verification though (doesn't have to be in this commit). There are various other ambiguities that it'd be nice if they were simple to add the core checks for (e.g. attr-dict next to a region).

In D125724#3517058, @Mogball wrote:

There's a further wrinkle here: attribute parsers embedded in operation assembly formats won't be able to have types.

def MyAttr : ... {
  let parameters = (ins "int":$a, AttributeSelfTypeParameter<"">:$type);
  let assemblyFormat = "$a";
}

def MyOp : ... {
  let arguments = (ins MyAttr:$a);
  let assemblyFormat = "$a attr-dict"
}

The pretty format won't be able to have a type because the parser directly invokes MyAttr::parse and won't be able to look ahead for the type.

Thanks!

Nice! Can you look into the flang test failures?

The test failures look real here, can you take a look? Looks like this doesn't properly handle the non-const case.

Was there somewhere in MLIR that could take advantage of this? Trying to recall from another patch.

Sweet, thanks!

Thanks all for the review!

Nice!

We should have some test dialect operations that use default dialect, should be easy to use a test dialect operation with multiple dots within the regions of one of those operations.

@bzcheeseman Can you advise if this is the easiest way to do a "self" cast?

Yeah, I'm not sure who would be good to review the actual json itself. Mostly just wondering if I should care about the existing xml package or not, happy to land this and not care about the other thing.

Can you also add documentation on how to add/define a purely dynamic dialect?

In D124023#3509442, @krzysz00 wrote:

( @rriddle To bring the signed/unsigned/ConstantRange discussion out of inline comments)

I've got a few reasons I'd say it's reasonable to track the signed and unsigned interpretations of values separately, in contrast with LLVM's approach.

1. The LLVM approach appears to rely on having a bunch of flow-sensitive reasoning that generates things like nsw in order to make their analysis more precise. We don't have, and probably don't want to recreate, all that infrastructure, especially with how extensible MLIR is. So, we need another avenue to get analysis precision.
2. Having signed/unsigned bounds like this makes it easier to reason about what the range inference implementations are doing, especially when dealing with mixtures of signed and unsigned ops (like divui or remsi). Trying to precisely get bounds out of mixed code like that is tricky.
3. In most cases, we don't have double computation - the three ops that really have to do duplicate reasoning about the signed and unsigned result are addition, subtraction, and multiplication. Just about everything else can call fromSigned or fromUnsigned after computing one set of bounds to copy the bounds over if that's a valid thing to do.
4. Initial attempts at a one-range system were a confusing mess and I'm not looking to fight through making sure that's implemented correctly.

Here is a quick screencap of PDL before and after:

I validated the results using most of our upstream test cases. I may have missed some potential edge cases, but this is a much better iteration point that what we had before. There are a few places where we miscolor, but that is mostly because it's impossible in some cases to differentiate a region from an attribute dictionary.

Yep. I'm waiting for the design to settle out on the lower layers before reviewing this one.