- User Since
- Jun 28 2016, 7:50 PM (195 w, 6 d)
Mon, Mar 23
Thank you for the review River!
Patch landed here, thanks!
Fri, Mar 20
Incorporate all the great suggestions from River's review.
Thank you so much for the fantastic review River! I've incorporated all of the suggested changes. I'll wait for Mehdi's ack before landing since he expressed concerns on the discourse thread. Thanks!
Fix a bug that Mehdi noticed in patch review in getResultName(). Expand
the testcase to handle and show the multiresult case explicitly, testing
the fix. Add a new OpAsmParser::getNumResults() method.
Thank you again so much for noticing this bug Mehdi. I've expanded out the test dialect op to be more interesting (allowing testing the multi-result case). I believe this is ready for review again, thanks!
Thu, Mar 19
Wed, Mar 18
Ok, this patch is good to go, @rriddle can you take a look when you get a chance?
Fix a clang tidy warning, fix the memory issue that caused it to fail on some builders not others.
Remove the test logic. This patch is good to review.
Change to raw_string_ostream to see if it fixes the linux builder, also
add some debug info to catch the problem if it doesn't work.
Sun, Mar 15
Ok, build 53570 looks like it has two unrelated failures, but the parser.mlir failure does look like a problem. This:
Thu, Mar 12
I'm not a qualified reviewer for this at this point.
Wed, Mar 11
River, does this patch LGTY? It is pretty straight-forward.
Tue, Mar 10
Also, what about using a string as the escape mechanism?
Who should own the complexity (and the definition of what a valid asm name is): the core, or all clients?
Thu, Mar 5
Avoid warning for other compilers.
Wed, Mar 4
Tue, Mar 3
Cool. Do you have time to review this patch for Ehud? he's been very patiently pinging
This question would be best for River. MLIR is using a simpler approach, it isn't clear to me whether Waymarking would be worth it, but it should be possible to benchmark it now.
Feb 28 2020
I'd be happy to help fix that problem. Please take a look at the llvm developer policy. :-)
Feb 27 2020
Seems fine to me.
+River in case you're interested in this.
Feb 19 2020
I'm super excited to see this progress towards supporting 'asm goto' with results! Great work!
Feb 15 2020
I haven't carefully reviewed the patch, but I think this is the right thing to do architecturally for the compiler. Thank you for driving this Reid. I'd appreciate it if someone could scrutinize the patch though!
Feb 13 2020
Very nice, please correct the CMAKE variable or, better yet, just drop the explicit mention of the flag to pass). I really appreciate that you are improving our policies!
Jan 12 2020
This looks great!
Jan 9 2020
The big question here, of course, is "do we want to do this?", and if we do, "do we want to do this now?".
Jan 7 2020
I'm not a comptent reviewer on the details for this whole patch, but the general intrinsic approach LGTM!
Dec 26 2019
This looks really great to me, thank you for driving this! I'd recommend giving this a couple of days for others to comment (as the doc itself suggest :-), given the global nature of the change. Thanks!
Dec 23 2019
Dec 19 2019
What's the advantage of a redundant way to represent the same thing? LLVM could have a 'sizeof' ConstantExpr for example, but never did (people use the same gep trick). It is generally good to have fewer more canonical ways to represent a thing if possible.
Dec 18 2019
My current thinking for shuffles is that we shouldn't represent the shuffle mask of shufflevector as a Constant at all. Currently, we basically treat it as an ArrayRef<int> anyway
This is a very clever approach, I agree it should work - nice job! That said, I don't think it follows that we should accept these constantexprs in a ShuffleVector mask.
Nov 29 2019
This LGTM but I'm not an active reviewer for this area, so I'd get a second opinion :)
Nov 3 2019
I'd recommend adding Steve Canon to APFloat related patches. Thanks!
Oct 24 2019
This looks like progress to me. I personally would go with size_t even despite the (theoretical IMO) concern about size_t being a different size, but either way works for me, and both are better than ulong.
Sep 10 2019
Awesome, thanks Eric! This is very useful for projects that want to use LLVM Support library but also care about codesize.
Sep 4 2019
Renaming this to LogAlignment is much more clear throughout the core infra, thank you! Please get someone who knows the Yaml stuff to approve as well.
Jul 18 2019
Jul 3 2019
I'm not a reviewer for LLD, but this change looks really great to me. The major thing to watch out for is that things like "XYZVar" need to be renamed to "xyzVar", but it looks like LLD was already spelling these cases as "XyzVar", so this doesn't come up.
Jun 14 2019
I commented on the llvmdev thread, but instead of moving this complexity around, I'd really rather see it go away. We should never have supported div/rem constant expressions in the first place...
Apr 25 2019
Apr 13 2019
Apr 7 2019
Apr 4 2019
Patch LGTM. I think that the isa_and_nonnull name is the least bad of the options that I've heard, and if someone comes up with a better name, we can always rename it in the future. Thank you for updating the docs as well!
Mar 19 2019
Mar 12 2019
this is a really great summary of the situation, thank you for collecting this in such a methodical way!
Feb 21 2019
I can understand Zach's position here, but LLDB has historically never conformed to the general LLVM naming or other conventions due to its heritage. It should not be taken as precedent that the rest of the project should follow.
Feb 19 2019
Changed recommendation for acronyms from lower case to upper case, as suggested by several responses to the RFC.
Feb 7 2019
I am very much +1 on this. That said, this isn't the sort of thing we just use patch review for. Please agitate a robust discussion about this on llvm-dev. :-)
Jan 18 2019
Fantastic, thanks for driving this.
Jan 17 2019
Jan 14 2019
I haven't reviewed the patch in full detail, but the predicate "comesBefore" should probably be something like "isBeforeInBlock".