+1 for david's comments, also please fix the clang-format thing of course.

Looks structurally good to me, but I'm not a domain expert here.

Nice, thank you River!

This looks fine to me in a quick review, but you're the best technical expert here Paul.

+1, thanks River

ok

Looking forward to this!!

nice!

This is so so so awesome, thank you for doing this!

Looks great, thanks Paul!

Thx for the quick review!

nice

It looks like I didn't file a PR about this

@jdd fyi

Awesome, thank you for implementing this. The documentation and implementation look really really great, I'm excited to see this River!

Nice, thanks darthscsi

LLVM uses "MAX_INT_BITS = (1<<24)-1". Unless there is a reason to be different, it would probably be good to align with that.

Fair enough, good point. In C, it is a function, whereas in tblgen it is a declaration. I'm ok dropping the parens.

Looks like a great feature to me! I'd recommend requiring the message, I'd also consider requiring parens around the condition and message. This makes it more C like and makes it look less like a type definition:

I'm ok with this, your expertise surpasses mine Paul :-)

Alternate approach is here: https://reviews.llvm.org/D93908

I'm preparing a patch to deabstract this, but it is ugly. The other middle-ground approach is to change ValueTypeIterator to do the null check.

This all looks reasonable to me Paul. One thing you can check to gain confidence is that the output of tblgen should be bitwise identical on all the generated .inc files.

thanks for the quick review, landed in 87c032f7b44

This is awesome, thank you!

Landed in 9eb3e564d3b, thanks!

Great catches, fixed. Thanks!

Fixes AlexZ recommended.

Yeah please don't do a macro. A comment should be enough.

this looks great to me Paul. I don't see the client that modifies the array, but such a client can do an explicit copy into its own std::vector

Yeah C++ syntax isn't great. If you take the path of returning arrayref, it is a bad idea to allow it to convert the a 'const&'. My understanding is that operator you found on arrayref will make a copy of the data (defeating the whole purpose of this patch :-), and that the lifetime will be bound to the statement. This means the reference will actually dangle. :-(

Yeah just let the return construct it. On the caller side, just say: auto x = Records.getAllDerivedDefinitions(..)

Did you consider returning ArrayRef<Record*> instead? This is a bit more of an abstract type. Both work though

This is really cool!

I'd look at git blame, see who contributed it. We can then contact them, or just propose deletion of the code on llvm-dev and see if anyone speaks up :)

This is really awesome River, I love adopting and dogfooding ODS features in the builtin/std dialect. Thanks for also fixing the TypeConstraint issue.

Looks reasonable to me, Paul mentions that only AVR is using it in tree.

Thanks Mehdi!

I can do that, but is there a reason to reject that?

nice, thank you!

LGTM thanks paul

Nice! StringMap is definitely a good way to go here, the timings in that talk don't look right at all.

Thx Sean!

the patch looks good to me, I think the most important thing here is to nail down the programmer manual dox to be airtight :-)

Woot! Go Paul!

But if we're in an unreachable block, then we allow other IR forms that are even less expected (self-reference), so we should loosen this restriction?

Oh ok, well I have no problem keeping it around in searchable tables, I misread this as making that more general.

Ah, I thought TypeOf_xxx was a transitionary thing that we'd drop over time (to reduce complexity). If that's the case, I'd recommend not documenting it.

LGTM, thanks Paul

I don't think this is the right way to go, this breaks the invariant that # inputs to a phi node should match the # predecessors to a block. I'd recommend allowing zero input phi nodes in zero-predecessor blocks. I'm not sure why we ban zero input phi nodes right now.

Huh interesting. I'd recommend making this a cl::opt flag so any client of timers can get it. Timer.cpp already has a bunch of these flags (e.g. TrackSpace) so I'd just add one more

Ah right, I suppose this will matter if/when the ODS support for custom types also provides a declarative way to specify asm printing/parsing.

Nice, thanks! Does this add a verifier style check that makes sure that inner type is an RTLValueType, or is that still the responsibility of the custom parser logic?

Looks reasonable to me Paul! The null backend is a great idea for analysis

I agree this is a bit redundant. I'd drop the two changes that I marked above in the diff, otherwise LGTM, thank you again!

Looks reasonable to me!

Looks great, but I'd add this to the top of the "adding a new thing" section above the "new target" section. Could you also revise the 'new target' to be defined in the terms you established? I think it makes sense to retain the majority of the text there, but aligning on terminology would be good.

Very nice Paul!

This look really really great Renato. After this lands, please also prepare a patch (~paragraph length) for the main DeveloperPolicy doc that gives the high level idea of what is going on, and links to this doc for more information.

Wow, very impressive regex-fu mehdi! It looks like one more pattern is needed, but amazing work!

very nice Paul, you're a machine :)

nice!

FWIW, I suggested interleave because of the llvm::interleave algorithm in llvm/ADT/STLExtras.h

The word "adjoin" is a bit obscure and I don't think it quote connotes the right thing here. How about !interleave instead?

Oops, thanks for the head's up @dblaikie !