- User Since
- Jun 18 2016, 2:24 PM (314 w, 2 d)
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)
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?
Thu, Jun 16
Wed, Jun 15
Mon, Jun 13
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.
Sun, Jun 12
I think this is in a good enough state that we can land and iterate on in-tree.
Sat, Jun 11
LG, mostly stylistic comments.
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.
Fri, Jun 10
The sharding here feels very arbitrary. Can we instead separate the operations based on what they are testing?
Thu, Jun 9
Wed, Jun 8
Tue, Jun 7
Really appreciate catching this and raising the discussion! (Approval for whenever you need/want to land)
Mon, Jun 6
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)
Sun, Jun 5
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?
Sat, Jun 4
Took a quick scan, will look more in detail tomorrow. Really appreciate the detailed documentation.
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.
Thu, Jun 2
Thanks! I really appreciate the patience during the review. This is really great work.