User Details
- User Since
- Jun 28 2016, 7:50 PM (239 w, 2 h)
Sun, Jan 24
+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.
Sat, Jan 23
Nice, thank you River!
Tue, Jan 19
This looks fine to me in a quick review, but you're the best technical expert here Paul.
Fri, Jan 15
+1, thanks River
ok
Looking forward to this!!
nice!
Tue, Jan 12
This is so so so awesome, thank you for doing this!
Mon, Jan 11
Looks great, thanks Paul!
Sun, Jan 10
Thx for the quick review!
Sat, Jan 9
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!
Wed, Jan 6
Nice, thanks darthscsi
Tue, Jan 5
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 :-)
Tue, Dec 29
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.
Mon, Dec 28
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
Dec 27 2020
This is awesome, thank you!
Dec 26 2020
Landed in 9eb3e564d3b, thanks!
Dec 23 2020
Dec 22 2020
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. :-(
Dec 21 2020
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
Dec 16 2020
This is really cool!
Dec 15 2020
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.
Dec 14 2020
Looks reasonable to me, Paul mentions that only AVR is using it in tree.
Dec 12 2020
Thanks Mehdi!
Dec 11 2020
I can do that, but is there a reason to reject that?
Dec 10 2020
nice, thank you!
Dec 7 2020
LGTM thanks paul
Dec 6 2020
Nice! StringMap is definitely a good way to go here, the timings in that talk don't look right at all.
Dec 5 2020
Dec 3 2020
Thx Sean!
Dec 2 2020
the patch looks good to me, I think the most important thing here is to nail down the programmer manual dox to be airtight :-)
Dec 1 2020
Woot! Go Paul!
Nov 29 2020
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?
Nov 28 2020
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.
Nov 26 2020
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
Nov 22 2020
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?
Nov 13 2020
Looks reasonable to me Paul! The null backend is a great idea for analysis
Nov 10 2020
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!
Nov 7 2020
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.
Nov 5 2020
Nov 3 2020
Wow, very impressive regex-fu mehdi! It looks like one more pattern is needed, but amazing work!
Nov 2 2020
very nice Paul, you're a machine :)
nice!
Nov 1 2020
FWIW, I suggested interleave because of the llvm::interleave algorithm in llvm/ADT/STLExtras.h
Oct 30 2020
The word "adjoin" is a bit obscure and I don't think it quote connotes the right thing here. How about !interleave instead?
Oct 27 2020
Oops, thanks for the head's up @dblaikie !