I would prefer if we could test this outside of the LLVM dialect if possible, given that it should just be a matter of using the Recursive type in the test dialect right?

Can you add documentation for this field in the AttributesAndTypes.md doc?

Nothing sticks out to me here as wrong/broken, but given this is affine stuff I'll defer to @bondhugula for approval.

This looks like a good starting point to land and build on. (discussion on RFC seemed positive as well)

Looks really great! Thanks for tackling this. (Sorry for the delay, coming back from two week OOO)

Looking great!

Can we have a test that checks that we no longer go into infinite recursion? Otherwise, LG!

Thanks for updating! (Sorry, coming back from OOO)

LG (I've been OOO for the past few weeks)

Can you improve the commit title/description? Right now it isn't super clear, e.g. maybe just: [mlir][NFC] Use proper c++ namespaces in .td files?

We don't need to touch the declarations in the .h at all?

In D127455#3574415, @mnadeem wrote:

In D127455#3573920, @rnk wrote:

Does the MLIR LLVM dialect not have comdats? That seems like a missing feature, a lack of fidelity that would prevent lossless conversion from LLVM IR to MLIR & back. Clang uses comdat groups for some esoteric features, the D5 destructor alias group, and MSVC RTTI.

I couldn't find a way to add comdats to MLIR (unless I missed something).

In D127537#3585984, @myhsu wrote:

In D127537#3583128, @Mogball wrote:

I'm not sure I understand what you're getting at. Mutability is tied to the abstract type/attribute implementation. Instead of abstractType->isMutable(), could we not have abstractType->hasTrait<TypeTrait::Mutable>()?

I see, I thought you were referring to "trait" as a general C++ concept rather than MLIR's Trait.

IIRC trait needs to be manually annotated on a type or attribute, while my current approach detects mutability out of the box. The reason I chose the current way was because TypeStorage/AttributeStorage use the same way to see if a type/attribute can be mutated. And I'm a little worry that using trait might make such property out-of-sync with TypeStorage/AttributeStorage, if, let's say, a dialect forgets to annotate their mutable types or attributes with trait.

LG, but please do not land patches that are marked as "Requested Changes".

I don't see a reason to regress from Attr/TypeDef using prefixed always, can we instead just fix the emission there to properly convert snake case?

This looks like a regression to me in many cases. We are trying to remove snake case, but this seems to be adding more uses of it.

I think this is in a good enough state that we can land and iterate on in-tree.

LG, mostly stylistic comments.

In D126352#3570062, @jpienaar wrote:

Sorry didn't get notification of comments.

In D126352#3537760, @Mogball wrote:

We should really be moving away from DialectType, can we move ShapeDialect types to ODS instead?

DialectType and DialectAttribute is only used for docgen at this point.

We could, I mostly just used these as an example of the other as was simple change. Would preference be leaving the shape dialect ones as is and just adding the helpers, or use the helpers an migrate them to ODS post? (It's not too many lines either way).

Yeah, the set should be unnecessary in 99% of cases. This is something that is only necessary for mutable attributes/types, given that those are the only ones that could be recursive. Conceptually we could detect mutability on registration, add a bit for that to the AbstractAttribute/Type classes, and check that here.

In D127522#3574569, @mehdi_amini wrote:

I agree with the fact that the test dialect should be sharded based on features instead of being a monolithic definition!

That said we need the sharding for our internal out-of-tree dialects still so it is a useful addition independently :)
(and the test dialect is just used to test the sharding feature)

To come back to the patch: what I'd like is the ability to change the sharding count in the CMake file without touching any source code anywhere. I just talked to Jeff about this and I think he has a solution for this!

In D127522#3574541, @Mogball wrote:

You mean moving ops into different dialects?

I foresee sadness and pain

In D127522#3574270, @rriddle wrote:

The sharding here feels very arbitrary. Can we instead separate the operations based on what they are testing? That feels like a much better cleanup, and a better end state than sharding.

The sharding here feels very arbitrary. Can we instead separate the operations based on what they are testing?

Really appreciate catching this and raising the discussion! (Approval for whenever you need/want to land)

In D127180#3562509, @Chia-hungDuan wrote:

In D127180#3562470, @rriddle wrote:

In D127180#3562404, @Chia-hungDuan wrote:

@rriddle
I'm not sure if we should do the check in AddToWorklist or just make it as this CL. Besides, I found this bug by the assertion we had before. Do you think we need a test (testing remove/replace/insert operations) or still have the assertion is enough?
What do you think?

Ahah, missed this comment before I sent the other one. What do you mean by if we should do the check in AddToWorklist? Also, I think it'd be nice to have a test for this if we can.

Sorry, didn't say that clearly. I mean things this like

virtual void AddToWorklist(...); // in GreedyPatternRewriteDriver
void AddToWorklist(...) override { // in MultiOpPatternRewriteDriver
  if (strictMode) ...
}

Then we may only need to override notifyOperationRemoved. Not sure which one is better in term of design.

In D127180#3562404, @Chia-hungDuan wrote:

@rriddle
I'm not sure if we should do the check in AddToWorklist or just make it as this CL. Besides, I found this bug by the assertion we had before. Do you think we need a test (testing remove/replace/insert operations) or still have the assertion is enough?
What do you think?

Is there a test we can add here?

The mlir tag in the commit title looks off, this is a change in polly (which is based on LLVMIR)

Thanks for looking into this! Have you considered/looked into using the same approach as LLVM? i.e. keeping an inline history to track what's already been inlined?

Took a quick scan, will look more in detail tomorrow. Really appreciate the detailed documentation.

In D126751#3558372, @Mogball wrote:

I think River's up next :)

But sure!

From a quick first pass we should really avoid building multiple analysis if we can. Each analysis is going to do full passes of the IR.

In D126446#3553562, @jfurtek wrote:

The current implementation uses hexadecimal, which requires 16 bits (2 bytes) to encode 8 bits of binary data. base64 would be an improvement over the current implementation that uses hexadecimal, requiring 8 bits to encode 6 bits of binary data. (The other constraint here is that encoding needs to be handled by the parser, which in the original example, means it can't have a quotation mark.) IMO, base64 is:

• smaller than hex
• somewhat ubiquitous
• compatible with the existing parser (I think?)
• a reasonable default for situations where the size overhead is an acceptable tradeoff for having the MLIR file be text-based and self-contained.

For text-based MLIR file situations where the size increase is an issue, I could imagine the config data containing a string with the name of an external file, and an application-defined way to resolve the full path. That could be out-of-tree and/or dialect specific.

Thanks!

Thanks! I really appreciate the patience during the review. This is really great work.

In D124023#3553211, @krzysz00 wrote:

I do want this functionality downstream for non-test reasons, though.

If it makes more sense, I can make -fold-inferred-constants a test pass in
the MLIR codebase and then temporarily pull it out of test/ in our fork
when I'm backporting this patch series. Would that work?

Or, if you don't think the SCCP evolution will be too contentious, I can
just split that into it's own patch and add it to the series - another few
days to make sure we get this right aren't the end of the world.

In D124023#3551660, @krzysz00 wrote:

I will admit I'm not entirely happy with the SCCP integration - if you see any ways I could tighten it up, please do let me know.

In D126444#3551698, @jpienaar wrote:

A part of the motivation that is missing here (but was mentioned in the RFC) is why do this at this level than in LLVM? E.g., type conversions for backends not supporting specific types is already in LLVM with all kinds of helpers of how one can generally lower by combining overflow behavior of compare odd with add with carry op (as example). It feels like one would need to reimplement some of those expansions here if done at this level. Given it was mentioned, was this discussed and evaluated?