Thank you for the review River!

Patch landed here, thanks!
https://github.com/llvm/llvm-project/commit/596da62d21ede197dd95eca5146d2ddf0497275c

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!

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.

Ok, build 53570 looks like it has two unrelated failures, but the parser.mlir failure does look like a problem. This:

Thanks River!

I'm not a qualified reviewer for this at this point.

River, does this patch LGTY? It is pretty straight-forward.

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?

Avoid warning for other compilers.

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.

I'd be happy to help fix that problem. Please take a look at the llvm developer policy. :-)

Hi Ehud,

Seems fine to me.

+River in case you're interested in this.

I'm super excited to see this progress towards supporting 'asm goto' with results! Great work!

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!

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!

This looks great!

The big question here, of course, is "do we want to do this?", and if we do, "do we want to do this now?".

In D71636#1813285, @efriedma wrote:

In D71636#1807522, @simoll wrote:

If shuffle masks are no longer IR entities, this means ruling out computed shuffle masks in the future, which are available on some targets (X86). Whether we want computed shuffle masks at all is a different question.

Yes, this is kind of a decision point: either we allow computed shufflemasks, or we get rid of the operand altogether. Or we can introduce some intrinsics and put off the decision, I guess.

I don't think there's much incentive to support computed shuffle masks. Yes, x86 has pshufb, but that doesn't generalize to other element widths/two source vectors/etc. easily. And there aren't very many practical use cases for computed shuffles in automatic vectorization.

If we allow computed shuffle masks, there's also the minor complication that we'd have to change shufflevector to produce poison, not undef, for undef indexes. But we can likely change that with some work...

I'm not a comptent reviewer on the details for this whole patch, but the general intrinsic approach LGTM!

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!

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.

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.

This LGTM but I'm not an active reviewer for this area, so I'd get a second opinion :)

I'd recommend adding Steve Canon to APFloat related patches. Thanks!

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.

Awesome, thanks Eric! This is very useful for projects that want to use LLVM Support library but also care about codesize.

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.

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.

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...

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!

In D57896#1434877, @Charusso wrote:

static Optional<const llvm::APSInt *>
getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
//...

if (const auto *DRE = dyn_cast_or_null<DeclRefExpr>(CondVarExpr)) {
  if (const auto *VD = dyn_cast_or_null<VarDecl>(DRE->getDecl())) {

//...
}

would be:

static Optional<const llvm::APSInt *> |
getConcreteIntegerValue(const Expr *cond_var_expr, const ExplodedNode *node) { |
//... |

if (const auto *decl_ref_expr = dyn_cast_or_null<DeclRefExpr>(cond_var_expr)) {
  if (const auto *var_decl = dyn_cast_or_null<VarDecl>(decl_ref_expr->getDecl())) {

//... |
} whoops column-81 ~^

Hungarian notation on members and globals are cool idea. However, the notation is made without the `_` part, so I think `mMember` is better than `m_member` as we used to 80-column standard and it is waste of space and hurts your C-developer eyes. I would recommend `b` prefix to booleans as Unreal Engine 4 styling is used to do that (`bIsCoolStyle`) and it is handy. It is useful because booleans usually has multiple prefixes: `has, have, is` and you would list all the booleans together in autocompletion. Yes, there is a problem: if the notation is not capital like the pure Hungarian notation then it is problematic to list and we are back to the `BIsCapitalLetter` and `MMember` CamelCase-world where we started (except one project). I think @lattner could say if it is useful as all the Apple projects based on those notations and could be annoying.

this is a really great summary of the situation, thank you for collecting this in such a methodical way!

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.

Changed recommendation for acronyms from lower case to upper case, as suggested by several responses to the RFC.

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. :-)

Fantastic, thanks for driving this.

Looks great!

I haven't reviewed the patch in full detail, but the predicate "comesBefore" should probably be something like "isBeforeInBlock".

Hi Reid,