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,

Looks great, thanks Chandler!

To be clear, I'm not necessarily saying that this patch is the wrong thing to do, I would just like more diligence and experimentation with alternate approaches. Thanks :-)

As I mentioned on llvmdev, I'm strongly opposed to this patch without a significant amount of analysis and diligence applied to it. I'm concerned about both memory and compile time impact. We don't just add random caches to the core IR to speed up particular clients. We care a lot about sizeof(Instruction) and its subclasses, and this adds bloat to all of them. Similarly, you don't seem interested in evaluating "tricks" to make this efficient in practice (sparse numbering), nor have you evaluated other implementation choices that will probably also work with less impact. Let's continue discussing this on llvm-dev.

Just a drive by, but I'm hugely +1 on this change, thank you for tackling it!

Patch LGTM, I'd suggest including an example like `auto updateResult = [&](){...`

nice!

How about changing it to "LLVM Compiler Infrastructure". Accepted with that tweak, thanks!

These changes look great, but I'd suggest explicitly mentioning that SNaN behavior somewhere, the relation of the instructions to the intrinsics etc. Perhaps a section before Fast Math Flags?

Here's a different way to interpret the current situation: instead of seeing the current behavior as "inconsistent", you could see it as "duplication only making sense in some cases". Realistically though, I agree that the code base is probably inconsistent as it is written by lots of people with different practices.

Personally, I don't see the harm in duplicating the comments, and they are useful when you're not using doxygen or some other tool. You're talking about the overriden method situation, but another common one is when you define a class in an anonymous namespace and then implement it later in the same file. The implementation should certainly have a doc comment on it, and if it helps to understand the overview of what the struct is doing, then it makes sense to document it in the class definition as well.

This change LGTM.

LGTM! Thanks @asb

FWIW, I agree with @majnemer's comments here: use of auto in those places doesn't aid in understanding/maintainability of the code, and it is important to mention that the manual loop with an explicitly evaluated end can make sense if the sequence is being mutated in the loop (but also we should recommend a comment to make that explicit!)

I agree we should remove it from the main page. DragonEgg should also be removed.

In D32737#760310, @sanjoy wrote:

I've only lightly read the spec, but it looks like the vector length can be controlled by writing to the ZCR_ELn registers (so, e.g. user code could make a syscall to change the vector length)? If that's accurate, I think a constant vscale is not sufficient.

In D32737#760373, @rengolin wrote:

In D32737#760289, @lattner wrote:

This is not a research project, it's a spec that is being made into hardware by a large number of large corporations and it's in development by a long number of years.

In D32737#760276, @chandlerc wrote:

In D32737#759325, @rengolin wrote:

@echristo @chandlerc @lattner @majnemer Ping.

This is a trivial change, discussed in the past, and I'm inclined to approve.

Uh, where was it discussed? It's entirely possible I missed it, but I can't find any consensus on any of the threads that we actually want to support runtime vector width in LLVM's IR.

...

For the record, I remain unconvinced that LLVM's IR should support non-constant vector widths

Thanks for improving this to allow programatic complexity! This looks overall great to me, but again, I'm not the best technical reviewer for X86 backend anymore.

I think there are two things going on here. I think that eli is right: LLVM IR global variables should always be assumable that they don't alias. This is guaranteed by C and other source languages and is important for LLVM to be able to represent. Separately though, I think it makes sense for LLVM globals to have !range metadata to specify that (though disjoint) they live in a specific address range that can fit into immediates.

This seems like a huge improvement over the status quo. Unfortunately, it's been a couple of years since I've worked on this area though, so I'd like you to get someone working on the X86 backend to look at it.

This doesn't seem like the right approach. It would be better to introduce a new top level IR concept to represent this, instead of violating the model of GlobalValue.

Ah, I didn't see it. I'll respond there.

As I mentioned on llvmdev, this approach doesn't make sense to me.

Looks reasonable, please add the standard LLVM file header blob per the coding standards.

I can't provide a technical review for this, but thank you for working on it!

This revision LGTM, making the "const char*" ctor explicit can & should be done as a second step, thanks!

You should be able to make string literals work if you use a template that takes an array of char with the size as a template parameter.

The 0 is for the length, not the actual pointer.

LGTM, might as well use nullptr instead of 0 though.