[Constants][SVE] Represent the runtime length of a scalable vector

[huntergr]

Summary

The length of a scalable vector is unknown during compilation. When
vectorising loops the runtime length is required to update induction
variables and thus a representation is required within the IR.

This patch introduces the 'vscale' identifier to represent the scaling
factor 'n' of a scalable vector of the form '<n x #elements x ty>'.

In use, induction variable updates for scalable vectorisation become:

; i += number_of_elements(<n x 4 x i32)
%i.next = add i32 %i, mul (i32 vscale, i32 4)

Picking up where Paul left off with https://reviews.llvm.org/D27103

[rengolin]

include/llvm-c/Core.h

1188

Not being in the same order as above may confuse people looking for it?

include/llvm/IR/Value.def

92

I don't get this change...

lib/IR/Constants.cpp

812

So, in theory, you can have vscale constans of different integer types, and this would only clear the ones that are the same as this one?

This sounds confusing.

[aemerson]

lib/IR/Constants.cpp

802

Indentation.

812

Yes, in the same way you can have i32 undef, i64 undef etc.

[huntergr]

include/llvm-c/Core.h

1188

These are already in a different order than the enum above -- Undef appears before ConstantInt there, but after it here. The indentation and the comment above it suggest a hierarchy, so I tried following that.

include/llvm/IR/Value.def

92

I added it at the end, after 'ConstantTokenNone', so needed to adjust the last marker. Since this isn't part of the C interface, I could add it before this and not need to change the markers.

[rengolin]

Hi Graham,

I have no more questions, but again, I'll let this one sit for other people to chime in, as it's a core change to IR.

cheers,
--renato

include/llvm-c/Core.h

1188

Makes sense.

include/llvm/IR/Value.def

92

Right, I see.

lib/IR/Constants.cpp

812

Right, makes sense.

[rengolin]

@echristo @chandlerc @lattner @majnemer Ping.

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

But given that it changes IR behaviour, I want to make sure everyone is on the same page.

thanks,
--renato

[chandlerc]

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.

The most recent thread I find on llvm-dev is from Mar 7: "[llvm-dev][RFC][SVE] Extend vector types to support SVE registers."

That thread exclusively talks about MVT and the code generator. I don't see its relevance to the IR.

Before taht we have the big RFC for SVE. And there, I had suggested in November of last year to get a fresh RFC which I don't see having happened yet.

And in response to that you indicated the patches under review were just examples, not planning to be committed.

If I missed the RFC, totally my bad, but I did search for 'SVE llvm-dev' and was unable to find it, so I suspect I may not be the only one that has continued to wait for an actual follow-up RFC.

For the record, I remain unconvinced that LLVM's IR should support non-constant vector widths. I understand why some CPU vendors are interested in this, but the motivation for LLVM to support it so far is very weak, and the cost in terms of complexity to the IR and every vector-aware optimization is, in my opinion, far too high. However, I'm trying to remain open about this subject and have been awaiting a fresh RFC on llvm-dev to really dig into the motivation.

[rengolin]

In D32737#760276, @chandlerc wrote:

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.

Hi Chandler,

This has been discussed in the list and phab, and I invited people from different targets and core devs to discuss. As usual, "consensus" is formed from the people that have actually cared, but there have been enough threads and discussions. We can always have more discussions, sure, but pulling the hand break without concrete proposals at this stage is really not fair.

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

Do you have an alternative implementation to scalable vectors?

I understand why some CPU vendors are interested in this, but the motivation for LLVM to support it so far is very weak, and the cost in terms of complexity to the IR and every vector-aware optimization is, in my opinion, far too high. However, I'm trying to remain open about this subject and have been awaiting a fresh RFC on llvm-dev to really dig into the motivation.

"Some CPU vendors"? Seriously?

This is not a thought experiment or an academic theoretical paper, ARM's SVE is out, many manufacturers were involved and the hardware is being developed right now, and we need to implement it somehow. RISC-V will have support for scalable vectors in the near future, not to mention GCC's ongoing implementation of the spec.

Not implementing SVE, in my opinion, is not an option. It's like not doing vector support because the new types will break scalar optimisations.

Given that this will be more common in the future, having a simple and (hopefully elegant) implementation will make it a lot easier for passes to infer semantics and maintain the existing optimisations working.

The MVT types are out already, other changes going and I'm really surprised at such comments at this stage.

--renato

[lattner]

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

I agree with Chandler on this. Making something a first class type in LLVM has wide reaching effects, including requiring the ability to load/store/phi the value, pass it by argument, etc. Vikram had a research project many many years ago to do the exact same sort of thing. It failed because of these and other reasons.

What are the semantics of select when the two vectors have different width? Does store do a memory allocation?

-Chris

[sanjoy]

In D32737#760289, @lattner wrote:

What are the semantics of select when the two vectors have different width? Does store do a memory allocation?

Maybe I misunderstood, but won't those selects be ill-typed?

lib/IR/Constants.cpp

812

Is there a minimum width, or is (say) an i1 vscale allowed? If there isn't a minimum, I presume the semantics is that the runtime value of vscale will be truncated to the type width?

[hfinkel]

In D32737#760292, @sanjoy wrote:

In D32737#760289, @lattner wrote:

What are the semantics of select when the two vectors have different width? Does store do a memory allocation?

Maybe I misunderstood, but won't those selects be ill-typed?

That is my understanding as well; the model is that there is one underlying vector width, we just don't know at compile time what it is. In any case, is there an overall LangRef patch? It might be best to clear up the semantics with an overall patch (even if we commit the changes in pieces along with the implementation).

[sanjoy]

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.

[rengolin]

In D32737#760289, @lattner wrote:

I agree with Chandler on this. Making something a first class type in LLVM has wide reaching effects, including requiring the ability to load/store/phi the value, pass it by argument, etc.

I don't think anyone disagrees with that point.

Vikram had a research project many many years ago to do the exact same sort of thing. It failed because of these and other reasons.

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.

There are already a number of production compilers (including LLVM based) that have SVE implemented in them.

I just want to make sure that we take the right *technical* decision now and evolve as needed. From my point of view, making scalable vectors a native and core type of IR is the only way forward, because the semantics needs to be ingrained in the language to make sense. AFAICS, at the IR level, the differences between vectors and scalable ones is not that big a deal, certainly not bigger than vectors versus scalar.

What are the semantics of select when the two vectors have different width?

As Sanjoy said, it's illegal behaviour. Vector length is a CPU runtime property, not a vector property. Vectors don't have length.

The IR should consider a scalable vector as a "promise of at least one iteration, but potentially more" of the same computation. The mask vectors do the rest of the job of making sure the semantics stays the same.

Does store do a memory allocation?

I'm not sure what you mean by that. Loads and stores do nothing more than what AVX512 (with scatter/gather) already does.

The main difference is that all operations (even non-scatter/gather) are predicated, so that there is absolute control over undefined behaviour. The predicate vector is updated from the scalar evolution of the iteration ranges and they control what the actual operations can and can't do, irrespective of the vector size.

cheers,
--renato

[rengolin]

lib/IR/Constants.cpp

812

The vscale does not define the vector length. That is defined by the CPU (via a status register) at runtime.

The *exact* same code can run in one process with length = 10 and another with length = 1. In theory, the same binary could run one instruction with 10 and the very next with 1 (that'd be crazy, but valid).

However, one instruction being executed by the unit will operate on identical lengths. Ie. you can't have two vectors of different sizes on the same "add". AFAIK this is not just illegal, it's theoretically impossible, from where that information comes from.

What's illegal (and probably traps) is if you set the status register to a value that is larger than the actual physical length, but that will never be generated by the compiler (which has no business setting the length at all), so it's not something the compiler should worry about.

[rengolin]

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)?

As I explained to Hal on his comment, that is correct but doesn't have the effect you're expecting.

Vectors don't have length, they have the "idea that they may have length", and it's up to the CPU to control that.

Just to be clear, the example you propose has no effect on the notion of length:

// SVE length defined at boot time to be 4
...
add z0.s, p0/m, z0.s, z1.s // z0+=z1 only where the predicate p0 is valid, which here is "up-to" 4 vector lengths
...
svc ... // Try to change vector length to 8, assuming this works
...
add z0.s, p0/m, z0.s, z1.s // z0+=z1 only where the predicate p0 is valid, which here is "up-to" 8 vector lengths

In the case above, the change in length applies to both z0 and z1, as well as p0 and all the other SVE vectors uniformly.

Using other SVE instructions, the predicate p0 will be built to go "up-to" the end of the array/memory as the semantics allows in IR, either compiler-constant or runtime check, so there's no compiler-generated undefined behaviour.

If that's accurate, I think a constant vscale is not sufficient.

The main problem here is one of representation. In the ARM implementation, SVE vectors alias with SIMD vectors, so you need to be careful on how you write to them.

If you don't have a way to separate SVE from SIMD, you'll have trouble generating either code. If you separate them completely, you'll have trouble worrying about the aliasing.

Having a flag (even as boolean "i1 vscale") is enough. It needs to be a constant because of how scalar evolution will work on the predicate vectors, but I'll let Graham explain that in more depth, as I'm only "familiar" at this point.

cheers,
--renato

[lattner]

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.

I'm familiar with it: I was involved in the work at apple (years ago) which contributed to SVE happening with this design.

I just want to make sure that we take the right *technical* decision now and evolve as needed.

This is obviously correct.

From my point of view, making scalable vectors a native and core type of IR is the only way forward, because the semantics needs to be ingrained in the language to make sense. AFAICS, at the IR level, the differences between vectors and scalable ones is not that big a deal, certainly not bigger than vectors versus scalar.

This is much more up for debate. Details matter. Compilers are a set of engineering tradeoffs. You need to explain and defend your position carefully, not just state it as though it were "obviously true".

Do you have a discussion somewhere of the overall design of the extensions you're proposing? This is presumably only a small piece of it.

Does store do a memory allocation?

I'm not sure what you mean by that. Loads and stores do nothing more than what AVX512 (with scatter/gather) already does.

You haven't proposed a set of IR type system extensions. If you're proposing a new first class type, one which represents a vector of an unknown length, then you'll need to be able to load and store it, e.g. to spill a virtual register. How much stack space is required for that spill?

The IR should consider a scalable vector as a "promise of at least one iteration, but potentially more" of the same computation. The mask vectors do the rest of the job of making sure the semantics stays the same.
The main difference is that all operations (even non-scatter/gather) are predicated, so that there is absolute control over undefined behaviour. The predicate vector is updated from the scalar evolution of the iteration ranges and they control what the actual operations can and can't do, irrespective of the vector size.

Seriously, I happen to be very familiar with the hardware/implementation model of this instruction set extension.

The thing that matters here is the specific set of IR extensions you're proposing. In this patch you're proposing introducing a vscale constant. This doesn't make sense, because it cannot be used in (e.g.) a global variable initializer, and otherwise doesn't fit the model for constants. Why not use an intrinsic to return this value?

-Chris

[rengolin]

In D32737#760430, @lattner wrote:

From my point of view, making scalable vectors a native and core type of IR is the only way forward, because the semantics needs to be ingrained in the language to make sense. AFAICS, at the IR level, the differences between vectors and scalable ones is not that big a deal, certainly not bigger than vectors versus scalar.

This is much more up for debate. Details matter. Compilers are a set of engineering tradeoffs. You need to explain and defend your position carefully, not just state it as though it were "obviously true".

I though that was clear from my previous response to you: "I don't think anyone disagrees with that point."

I'm certainly not asking people to "trust me, I'm an engineer". We had previous discussions in the list, other people chimed in. This is not even my patch and I have nothing to do with their work... "I just work here".

I'm also certainly not asserting that I know all the answers and that this change is worry free, by any means, and that's precisely why I pinged more people to give their opinions, because I was uncomfortable with the low amount of reviews. But it was certainly not "just me".

Do you have a discussion somewhere of the overall design of the extensions you're proposing? This is presumably only a small piece of it.

There were discussions on the list, a "whole-patch approach" in Github and some previous patches. I can't find any of it (my mail client - gmail - is a mess). I'll let Graham cover that part.

You haven't proposed a set of IR type system extensions. If you're proposing a new first class type, one which represents a vector of an unknown length, then you'll need to be able to load and store it, e.g. to spill a virtual register. How much stack space is required for that spill?

Ah, right. I'll let Graham take that one, as they have done this already in LLVM.

Seriously, I happen to be very familiar with the hardware/implementation model of this instruction set extension.

I meant no offence.

The thing that matters here is the specific set of IR extensions you're proposing. In this patch you're proposing introducing a vscale constant.

s/me/Graham/, this is not my patch and I had no part in any of this.

This doesn't make sense, because it cannot be used in (e.g.) a global variable initializer, and otherwise doesn't fit the model for constants. Why not use an intrinsic to return this value?

IIUC, it can be used for global variable initialiser via SVE splats using another new construct (stepvector) they want to introduce (which I had my own concerns).

I think the "stepvector" idea is limiting and could possibly start as an intrinsic, but the vscale is not really the actual scale, just a flag that it is scalable, so it wouldn't have more problems than using some intrinsic.

It'll still need a new register class in ISel and the register allocation, and it will still need to understand the aliasing rules, in the same way as we currently have for VFP and NEON.

If the vscale could make the stack size variable, so would an intrinsic. Or maybe I'm just not understanding the problem.

cheers,
--renato

[rengolin]

Just to be clear, I'm not *against* the idea of an intrinsic, nor I'm pushing this patch for any personal/professional agenda. I hope I have made that perfectly clear on my previous reviews on the same patches before.

I just want the best technical overall solution, and this particular one seems fine to me. I may be absolutely wrong, and that's perfectly fine, but we need a solution for this, even if we have to start with intrinsics and move to IR changes.

--renato

[hfinkel]

In D32737#760375, @rengolin wrote:

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)?

As I explained to Hal on his comment, that is correct but doesn't have the effect you're expecting.

Which comment? FWIW, I didn't see a particular response.

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.

I'm also under the impression that this won't work because it would interfere with any ongoing vector calculations, spill code, etc. The point being that it is fixed for a particular process once the process begins (at least in practice).

[sanjoy]

In D32737#760375, @rengolin wrote:

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)?

As I explained to Hal on his comment, that is correct but doesn't have the effect you're expecting.

Vectors don't have length, they have the "idea that they may have length", and it's up to the CPU to control that.

Just to be clear, the example you propose has no effect on the notion of length:

// SVE length defined at boot time to be 4
...
add z0.s, p0/m, z0.s, z1.s // z0+=z1 only where the predicate p0 is valid, which here is "up-to" 4 vector lengths
...
svc ... // Try to change vector length to 8, assuming this works
...
add z0.s, p0/m, z0.s, z1.s // z0+=z1 only where the predicate p0 is valid, which here is "up-to" 8 vector lengths

Let me try to give an example. Say we have code like (quasi-llvm syntax):

// vector length is 4
%v0 = load <4 x vscale x i8>, <4 x vscale x i8>* %ptr0
svc ... // Try to change vector length to 8, assuming this works
%ptr1 = %ptr0 + vscale
%v1 = load <4 x vscale x i8>, <4 x vscale x i8>* %ptr1
%v2 = add %v1, %v2

I have two questions here:

[edit: I just saw Hal's reply -- if we *disallow* mid-process changes to the vector length, then things are much simpler, but that needs to be documented.]

• What is the semantics of add %v1, %v2? As far as I can tell, the two vectors have "different" vector lengths, since one was created before the resize and the other was created after. I know the *registers* will have the same size, but as I understand it, one of the 32-byte registers will have 16 elements, while the other one will have 32. The bit that's worrying me here is that if we allow resizing operations then things like shufflevector (say) will have to be ordered with respect to unknown calls.
• Whether %ptr1 is computed before or after the syscall gives the program different semantics since it will either be 4 or 8 bytes after %ptr0. Does this mean we will have to order %ptr1 (a GEP) with respect to unknown function calls?

If that's accurate, I think a constant vscale is not sufficient.

The main problem here is one of representation. In the ARM implementation, SVE vectors alias with SIMD vectors, so you need to be careful on how you write to them.

If you don't have a way to separate SVE from SIMD, you'll have trouble generating either code. If you separate them completely, you'll have trouble worrying about the aliasing.

I'm not sure how SIMD etc. is related to what I asked.

Having a flag (even as boolean "i1 vscale") is enough. It needs to be a constant because of how scalar evolution will work on the predicate vectors, but I'll let Graham explain that in more depth, as I'm only "familiar" at this point.

Hm? I was under the impression that vscale was supposed to help offset induction variables (and things like that) by the right amount. How would you do that with an i1 vscale?

lib/IR/Constants.cpp

812

What I meant to say is, say I have code like:

for (iN i = 0; i < L; i += (iN vscale)) {
  load scaled vector from &a[i];
  ...
}

Does N have to be greater than some value for the loop above to make sense? For instance, if the vector length in the CPU is set to 32 then N = 2 clearly does not make sense -- i2 32 is just i2 0. If there is such a restriction, then it needs to be documented.

[rengolin]

In D32737#760457, @hfinkel wrote:

As I explained to Hal on his comment, that is correct but doesn't have the effect you're expecting.

Which comment? FWIW, I didn't see a particular response.

Sorry, not yours, Sanjoy's:

https://reviews.llvm.org/D32737#inline-290542

I'm also under the impression that this won't work because it would interfere with any ongoing vector calculations, spill code, etc. The point being that it is fixed for a particular process once the process begins (at least in practice).

Range calculations won't have to bother with the scale of the vector, as I'll try to explain on Sanjoy's reply later on.

Spill code may be problematic (variable stack), but that's a problem that I'm not sure we can fix with any notation.

[chandlerc]

There is a lot of discussion here that I really don't think should be on a patch review. It should be an an llvm-dev thread. See below.

In D32737#760433, @rengolin wrote:

In D32737#760430, @lattner wrote:

From my point of view, making scalable vectors a native and core type of IR is the only way forward, because the semantics needs to be ingrained in the language to make sense. AFAICS, at the IR level, the differences between vectors and scalable ones is not that big a deal, certainly not bigger than vectors versus scalar.

This is much more up for debate. Details matter. Compilers are a set of engineering tradeoffs. You need to explain and defend your position carefully, not just state it as though it were "obviously true".

I though that was clear from my previous response to you: "I don't think anyone disagrees with that point."

I'm certainly not asking people to "trust me, I'm an engineer". We had previous discussions in the list, other people chimed in. This is not even my patch and I have nothing to do with their work... "I just work here".

I'm also certainly not asserting that I know all the answers and that this change is worry free, by any means, and that's precisely why I pinged more people to give their opinions, because I was uncomfortable with the low amount of reviews. But it was certainly not "just me".

Do you have a discussion somewhere of the overall design of the extensions you're proposing? This is presumably only a small piece of it.

There were discussions on the list, a "whole-patch approach" in Github and some previous patches. I can't find any of it (my mail client - gmail - is a mess). I'll let Graham cover that part.

Yes, but in that discussion, I specifically asked for a new, fresh RFC thread that reflected substantial changes made to the design presented in the first RFC email through the discussion. That thread, AFAICT, never happened. If I missed, it I'm happy to get a pointer to it. If not, the author of this patch should start it. Either way, that is where this discussion should take place. I think there remain a lot of important technical issues here. I would like to dig into them, but I *don't* want to do it here where I don't even have the complete design.

I would encourage everyone (Hal, Sanjoy, etc) to hold off on debating "how does X work" here and redirect that to the (either existing or eventual) llvm-dev thread with an updated overall design.

[chandlerc]

In D32737#760472, @chandlerc wrote:

There is a lot of discussion here that I really don't think should be on a patch review. It should be an an llvm-dev thread. See below.

In D32737#760433, @rengolin wrote:

In D32737#760430, @lattner wrote:

From my point of view, making scalable vectors a native and core type of IR is the only way forward, because the semantics needs to be ingrained in the language to make sense. AFAICS, at the IR level, the differences between vectors and scalable ones is not that big a deal, certainly not bigger than vectors versus scalar.

This is much more up for debate. Details matter. Compilers are a set of engineering tradeoffs. You need to explain and defend your position carefully, not just state it as though it were "obviously true".

I though that was clear from my previous response to you: "I don't think anyone disagrees with that point."

I'm certainly not asking people to "trust me, I'm an engineer". We had previous discussions in the list, other people chimed in. This is not even my patch and I have nothing to do with their work... "I just work here".

I'm also certainly not asserting that I know all the answers and that this change is worry free, by any means, and that's precisely why I pinged more people to give their opinions, because I was uncomfortable with the low amount of reviews. But it was certainly not "just me".

Do you have a discussion somewhere of the overall design of the extensions you're proposing? This is presumably only a small piece of it.

There were discussions on the list, a "whole-patch approach" in Github and some previous patches. I can't find any of it (my mail client - gmail - is a mess). I'll let Graham cover that part.

Yes, but in that discussion, I specifically asked for a new, fresh RFC thread that reflected substantial changes made to the design presented in the first RFC email through the discussion. That thread, AFAICT, never happened. If I missed, it I'm happy to get a pointer to it. If not, the author of this patch should start it. Either way, that is where this discussion should take place. I think there remain a lot of important technical issues here. I would like to dig into them, but I *don't* want to do it here where I don't even have the complete design.

I would encourage everyone (Hal, Sanjoy, etc) to hold off on debating "how does X work" here and redirect that to the (either existing or eventual) llvm-dev thread with an updated overall design.

Put differently, this patch makes sense *once* we clearly have consensus on llvm-dev. So far, the only thread I can find did not reach any meaningful consensus. Notably, none of the code generator people were heavily contributing to that thread, and there remain large unmentioned technical concerns (stack spills, alloca size, etc)

[lattner]

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.

@rengolin is correct. The only sensible way to model this feature is with the vector length being a (load time) constant. Changing while a process is executing is not a useful thing to model or worry about. I still think this is better modeled with an intrinsic that returns the value, rather than and llvm::Constant.

[rengolin]

In D32737#760473, @chandlerc wrote:

Put differently, this patch makes sense *once* we clearly have consensus on llvm-dev. So far, the only thread I can find did not reach any meaningful consensus. Notably, none of the code generator people were heavily contributing to that thread, and there remain large unmentioned technical concerns (stack spills, alloca size, etc)

I agree. I'm glad I pushed this further, and I think we should get a proper discussion in the list.

In my mind, the previous discussion was "good enough", because all my questions were answered and I saw that other people weren't complaining much (so I assumed everyone was happy).

When you reported on the actual status I could see that I was probably inside a bubble. Initially, the idea was to start slow and "cross bridges when we get there", but changing the IR is really serious.

Graham,

I think we'll need an RFC that goes beyond vscale. We need to understand how constants are handles, as well as scalar evolution, spills, stacks etc. Not to work on the patches or even publish them now, just to understand a few cases in IR.

It would be easier to see the proposal, and then have a counter-proposal using intrinsics, to see how things will look like.

I know ARM was initially reluctant to use intrinsics, but as I said before on previous reviews, they allow us to model the behaviour before any hard changes to IR. If we can't reach a consensus now, it'd probably be better to go that way for now and change as it becomes clearer what to do in IR.

In the long run, I still think we should support scalable vectors native in IR, but that can wait until everyone understands the actual semantics.

cheers,
--renato

[aemerson]

Hi all,

There are good questions here which we'll enumerate and answer individually once we send out a new RFC to llvm-dev.

The reason that we didn't send out a new RFC to the list and instead sent patches was because the basic idea of our SVE implementation was fundamentally unchanged in terms the modified llvm::VectorType. The other elements of our implementation didn't affect this core concept, e.g. whether we used an instruction like elementcount or our new vscale constant to deal with runtime vector lengths, likewise with the stepvector constant vs seriesvector instruction.

To clarify again, from the compiler's perspective we can assume that the VL is constant but unknown. If you have any other questions ping them to us and we'll try to answer them as part of the new RFC.

Amara

[huntergr]

New RFC posted to llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2017-June/113587.html

[huntergr]

Outdated

docs/LangRef.rst

**Zero initialization**
     is always exactly equivalent to using explicit zero initializers.
 **Metadata node**
     A metadata node is a constant tuple without types. For example:
     "``!{!0, !{!2, !0}, !"test"}``". Metadata can reference constant values,
     for example: "``!{!0, i32 0, i8* @global, i64 (i64)* @function, !"str"}``".
     Unlike other typed constants that are meant to be interpreted as part of
     the instruction stream, metadata is a place to attach additional
     information such as debug info.
+**Scalable Vector Length**
+  Scalable vectors take the form "``<m x <# elements> x <elementtype> >``",
+  where "``m``" is an unknown positive integer constant. The string
+  "``vscale``" can be used to reference this value wherever an
+  :ref:`integer <t_integer>` is expected.
 
 Global Variable and Function Addresses
 --------------------------------------
 
 The addresses of :ref:`global variables <globalvars>` and
 :ref:`functions <functionstructure>` are always implicitly valid
 (link-time) constants. These constants are explicitly referenced when
 the :ref:`identifier for the global <identifiers>` is used and always have

include/llvm-c/Core.h

typedef enum {
   LLVMConstantFPValueKind,
   LLVMConstantPointerNullValueKind,
   LLVMConstantTokenNoneValueKind,
 
   LLVMMetadataAsValueValueKind,
   LLVMInlineAsmValueKind,
 
   LLVMInstructionValueKind,
+  LLVMVScaleValueValueKind,
 } LLVMValueKind;
 
 typedef enum {
   LLVMIntEQ = 32, /**< equal */
   LLVMIntNE,      /**< not equal */
   LLVMIntUGT,     /**< unsigned greater than */
   LLVMIntUGE,     /**< unsigned greater or equal */
   LLVMIntULT,     /**< unsigned less than */
macro(Constant)                         \
       macro(ConstantTokenNone)              \
       macro(ConstantVector)                 \
       macro(GlobalValue)                    \
         macro(GlobalAlias)                  \
         macro(GlobalObject)                 \
           macro(Function)                   \
           macro(GlobalVariable)             \
       macro(UndefValue)                     \
+      macro(VScaleValue)                    \
     macro(Instruction)                      \
       macro(BinaryOperator)                 \
       macro(CallInst)                       \
         macro(IntrinsicInst)                \
           macro(DbgInfoIntrinsic)           \
             macro(DbgDeclareInst)           \
           macro(MemIntrinsic)               \
             macro(MemCpyInst)               \

include/llvm/Bitcode/LLVMBitCodes.h

CST_CODE_INLINEASM_OLD = 18,   // INLINEASM:     [sideeffect|alignstack,
                                  //                 asmstr,conststr]
   CST_CODE_CE_SHUFVEC_EX = 19,   // SHUFVEC_EX:    [opty, opval, opval, opval]
   CST_CODE_CE_INBOUNDS_GEP = 20, // INBOUNDS_GEP:  [n x operands]
   CST_CODE_BLOCKADDRESS = 21,    // CST_CODE_BLOCKADDRESS [fnty, fnval, bb#]
   CST_CODE_DATA = 22,            // DATA:          [n x elements]
   CST_CODE_INLINEASM = 23,       // INLINEASM:     [sideeffect|alignstack|
                                  //                 asmdialect,asmstr,conststr]
   CST_CODE_CE_GEP_WITH_INRANGE_INDEX = 24, //      [opty, flags, n x operands]
+  CST_CODE_VSCALE = 25,          // VSCALE
 };
 
 /// CastOpcodes - These are values used in the bitcode files to encode which
 /// cast a CST_CODE_CE_CAST or a XXX refers to.  The values of these enums
 /// have no fixed relation to the LLVM IR enum values.  Changing these will
 /// break compatibility with old files.
 enum CastOpcodes {
   CAST_TRUNC = 0,

include/llvm/IR/Constants.h

public:
   unsigned getNumElements() const;
 
   /// Methods for support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const Value *V) {
     return V->getValueID() == UndefValueVal;
   }
 };
 
+//===----------------------------------------------------------------------===//
+/// A constant representing the scaling factor 'm' of a scalable vector of the
+/// form '<m x #elements x ty>'.
+///
+class VScaleValue final : public ConstantData {
+  VScaleValue(const VScaleValue &) = delete;
+
+  friend class Constant;
+  void destroyConstantImpl();
+
+  explicit VScaleValue(Type *T) : ConstantData(T, VScaleValueVal) {}
+
+public:
+  /// Static factory methods - Return a 'vscale' object of the specified type.
+  static Constant *get(Type *T);
+
+  /// Methods for support type inquiry through isa, cast, and dyn_cast:
+  static bool classof(const Value *V) {
+    return V->getValueID() == VScaleValueVal;
+  }
+};
+
 } // end namespace llvm
 
 #endif // LLVM_IR_CONSTANTS_H

include/llvm/IR/Value.def

 HANDLE_CONSTANT(UndefValue)
 HANDLE_CONSTANT(ConstantAggregateZero)
 HANDLE_CONSTANT(ConstantDataArray)
 HANDLE_CONSTANT(ConstantDataVector)
 HANDLE_CONSTANT(ConstantInt)
 HANDLE_CONSTANT(ConstantFP)
 HANDLE_CONSTANT(ConstantPointerNull)
 HANDLE_CONSTANT(ConstantTokenNone)
+HANDLE_CONSTANT(VScaleValue)
 
 HANDLE_METADATA_VALUE(MetadataAsValue)
 HANDLE_INLINE_ASM_VALUE(InlineAsm)
 
 HANDLE_INSTRUCTION(Instruction)
 // Enum values starting at InstructionVal are used for Instructions;
 // don't add new values here!
 
 HANDLE_CONSTANT_MARKER(ConstantFirstVal, Function)
-HANDLE_CONSTANT_MARKER(ConstantLastVal, ConstantTokenNone)
+HANDLE_CONSTANT_MARKER(ConstantLastVal, VScaleValue)
 HANDLE_CONSTANT_MARKER(ConstantDataFirstVal, UndefValue)
-HANDLE_CONSTANT_MARKER(ConstantDataLastVal, ConstantTokenNone)
+HANDLE_CONSTANT_MARKER(ConstantDataLastVal, VScaleValue)
 HANDLE_CONSTANT_MARKER(ConstantAggregateFirstVal, ConstantArray)
 HANDLE_CONSTANT_MARKER(ConstantAggregateLastVal, ConstantVector)
 
 #undef HANDLE_GLOBAL_VALUE
 #undef HANDLE_CONSTANT
 #undef HANDLE_INSTRUCTION
 #undef HANDLE_METADATA_VALUE
 #undef HANDLE_INLINE_ASM_VALUE
 #undef HANDLE_VALUE
 #undef HANDLE_CONSTANT_MARKER

lib/AsmParser/LLLexer.cpp

#define KEYWORD(STR)                                                           \
   KEYWORD(extern_weak);
   KEYWORD(external);
   KEYWORD(thread_local);
   KEYWORD(localdynamic);
   KEYWORD(initialexec);
   KEYWORD(localexec);
   KEYWORD(zeroinitializer);
   KEYWORD(undef);
+  KEYWORD(vscale);
   KEYWORD(null);
   KEYWORD(none);
   KEYWORD(to);
   KEYWORD(caller);
   KEYWORD(within);
   KEYWORD(from);
   KEYWORD(tail);
   KEYWORD(musttail);

lib/AsmParser/LLParser.h

enum {
       t_LocalID, t_GlobalID,           // ID in UIntVal.
       t_LocalName, t_GlobalName,       // Name in StrVal.
       t_APSInt, t_APFloat,             // Value in APSIntVal/APFloatVal.
       t_Null, t_Undef, t_Zero, t_None, // No value.
       t_EmptyArray,                    // No value:  []
       t_Constant,                      // Value in ConstantVal.
       t_InlineAsm,                     // Value in FTy/StrVal/StrVal2/UIntVal.
       t_ConstantStruct,                // Value in ConstantStructElts.
-      t_PackedConstantStruct           // Value in ConstantStructElts.
+      t_PackedConstantStruct,          // Value in ConstantStructElts.
+      t_VScale                         // No value.
     } Kind = t_LocalID;
 
     LLLexer::LocTy Loc;
     unsigned UIntVal;
     FunctionType *FTy = nullptr;
     std::string StrVal, StrVal2;
     APSInt APSIntVal;
     APFloat APFloatVal{0.0};

lib/AsmParser/LLParser.cpp

case lltok::kw_true:
     ID.Kind = ValID::t_Constant;
     break;
   case lltok::kw_false:
     ID.ConstantVal = ConstantInt::getFalse(Context);
     ID.Kind = ValID::t_Constant;
     break;
   case lltok::kw_null: ID.Kind = ValID::t_Null; break;
   case lltok::kw_undef: ID.Kind = ValID::t_Undef; break;
+  case lltok::kw_vscale: ID.Kind = ValID::t_VScale; break;
   case lltok::kw_zeroinitializer: ID.Kind = ValID::t_Zero; break;
   case lltok::kw_none: ID.Kind = ValID::t_None; break;
 
   case lltok::lbrace: {
     // ValID ::= '{' ConstVector '}'
     Lex.Lex();
     SmallVector<Constant*, 16> Elts;
     if (ParseGlobalValueVector(Elts) ||
if (StructType *ST = dyn_cast<StructType>(Ty)) {
           return Error(ID.Loc, "element " + Twine(i) +
                     " of struct initializer doesn't match struct element type");
 
       V = ConstantStruct::get(
           ST, makeArrayRef(ID.ConstantStructElts.get(), ID.UIntVal));
     } else
       return Error(ID.Loc, "constant expression type mismatch");
     return false;
+  case ValID::t_VScale:
+    if (!Ty->isIntegerTy())
+      return Error(ID.Loc, "vscale must be an integer type");
+    V = VScaleValue::get(Ty);
+    return false;
   }
   llvm_unreachable("Invalid ValID");
 }
 
 bool LLParser::parseConstantValue(Type *Ty, Constant *&C) {
   C = nullptr;
   ValID ID;
   auto Loc = Lex.getLoc();
   if (ParseValID(ID, /*PFS=*/nullptr))
     return true;
   switch (ID.Kind) {
   case ValID::t_APSInt:
   case ValID::t_APFloat:
   case ValID::t_Undef:
   case ValID::t_Constant:
   case ValID::t_ConstantStruct:
-  case ValID::t_PackedConstantStruct: {
+  case ValID::t_PackedConstantStruct:
+  case ValID:: t_VScale: {
     Value *V;
     if (ConvertValIDToValue(Ty, ID, V, /*PFS=*/nullptr))
       return true;
     assert(isa<Constant>(V) && "Expected a constant value");
     C = cast<Constant>(V);
     return false;
   }
   case ValID::t_Null:

lib/AsmParser/LLToken.h

enum Kind {
   kw_extern_weak,
   kw_external,
   kw_thread_local,
   kw_localdynamic,
   kw_initialexec,
   kw_localexec,
   kw_zeroinitializer,
   kw_undef,
+  kw_vscale,
   kw_null,
   kw_none,
   kw_to,
   kw_caller,
   kw_within,
   kw_from,
   kw_tail,
   kw_musttail,

lib/Bitcode/Reader/BitcodeReader.cpp

while (true) {
     Type *VoidType = Type::getVoidTy(Context);
     Value *V = nullptr;
     unsigned BitCode = Stream.readRecord(Entry.ID, Record);
     switch (BitCode) {
     default:  // Default behavior: unknown constant
     case bitc::CST_CODE_UNDEF:     // UNDEF
       V = UndefValue::get(CurTy);
       break;
+    case bitc::CST_CODE_VSCALE:    // VSCALE
+      V = VScaleValue::get(CurTy);
+      break;
     case bitc::CST_CODE_SETTYPE:   // SETTYPE: [typeid]
       if (Record.empty())
         return error("Invalid record");
       if (Record[0] >= TypeList.size() || !TypeList[Record[0]])
         return error("Invalid record");
       if (TypeList[Record[0]] == VoidType)
         return error("Invalid constant type");
       CurTy = TypeList[Record[0]];

lib/Bitcode/Writer/BitcodeWriter.cpp

for (unsigned i = FirstVal; i != LastVal; ++i) {
     }
     const Constant *C = cast<Constant>(V);
     unsigned Code = -1U;
     unsigned AbbrevToUse = 0;
     if (C->isNullValue()) {
       Code = bitc::CST_CODE_NULL;
     } else if (isa<UndefValue>(C)) {
       Code = bitc::CST_CODE_UNDEF;
+    } else if (isa<VScaleValue>(C)) {
+      Code = bitc::CST_CODE_VSCALE;
     } else if (const ConstantInt *IV = dyn_cast<ConstantInt>(C)) {
       if (IV->getBitWidth() <= 64) {
         uint64_t V = IV->getSExtValue();
         emitSignedInt64(Record, V);
         Code = bitc::CST_CODE_INTEGER;
         AbbrevToUse = CONSTANTS_INTEGER_ABBREV;
       } else {                             // Wide integers, > 64 bits in size.
         // We have an arbitrary precision integer value to write whose

lib/IR/AsmWriter.cpp

if (isa<ConstantTokenNone>(CV)) {
     return;
   }
 
   if (isa<UndefValue>(CV)) {
     Out << "undef";
     return;
   }
 
+  if (isa<VScaleValue>(CV)) {
+    Out << "vscale";
+    return;
+  }
+
   if (const ConstantExpr *CE = dyn_cast<ConstantExpr>(CV)) {
     Out << CE->getOpcodeName();
     WriteOptimizationInfo(Out, CE);
     if (CE->isCompare())
       Out << ' ' << CmpInst::getPredicateName(
                         static_cast<CmpInst::Predicate>(CE->getPredicate()));
     Out << " (";
 

lib/IR/Constants.cpp

 unsigned UndefValue::getNumElements() const {
   Type *Ty = getType();
   if (auto *ST = dyn_cast<SequentialType>(Ty))
     return ST->getNumElements();
   return Ty->getStructNumElements();
 }
 
 //===----------------------------------------------------------------------===//
+//                        VScaleValue Implementation
+//===----------------------------------------------------------------------===//
+
+Constant *VScaleValue::get(Type *Ty) {
+  assert(Ty->isIntegerTy() && "VScale must be an integer type!");
+
+  std::unique_ptr<VScaleValue> &Entry =
+  Ty->getContext().pImpl->VSVConstants[Ty];
+  if (!Entry)
+    Entry.reset(new VScaleValue(Ty));
+
+  return Entry.get();
+}
+
+/// Remove the constant from the constant table.
+void VScaleValue::destroyConstantImpl() {
+  // Free the constant and any dangling references to it.
+  getContext().pImpl->VSVConstants.erase(getType());
+}
+
+//===----------------------------------------------------------------------===//
 //                            ConstantXXX Classes
 //===----------------------------------------------------------------------===//
 
 template <typename ItTy, typename EltTy>
 static bool rangeOnlyContains(ItTy Start, ItTy End, EltTy Elt) {
   for (; Start != End; ++Start)
     if (*Start != Elt)
       return false;

lib/IR/LLVMContextImpl.h

#include "llvm/IR/Metadata.def"
   StructConstantsTy StructConstants;
   
   typedef ConstantUniqueMap<ConstantVector> VectorConstantsTy;
   VectorConstantsTy VectorConstants;
 
   DenseMap<PointerType *, std::unique_ptr<ConstantPointerNull>> CPNConstants;
 
   DenseMap<Type *, std::unique_ptr<UndefValue>> UVConstants;
+  DenseMap<Type *, std::unique_ptr<VScaleValue>> VSVConstants;
 
   StringMap<ConstantDataSequential*> CDSConstants;
 
   DenseMap<std::pair<const Function *, const BasicBlock *>, BlockAddress *>
     BlockAddresses;
   ConstantUniqueMap<ConstantExpr> ExprConstants;
 
   ConstantUniqueMap<InlineAsm> InlineAsms;

lib/IR/LLVMContextImpl.cpp

#include "llvm/IR/Metadata.def"
   ArrayConstants.freeConstants();
   StructConstants.freeConstants();
   VectorConstants.freeConstants();
   InlineAsms.freeConstants();
 
   CAZConstants.clear();
   CPNConstants.clear();
   UVConstants.clear();
+  VSVConstants.clear();
   IntConstants.clear();
   FPConstants.clear();
 
   for (auto &CDSConstant : CDSConstants)
     delete CDSConstant.second;
   CDSConstants.clear();
 
   // Destroy attributes.

test/Bitcode/compatibility.ll

 @const.false = constant i1 false
 ; CHECK: @const.false = constant i1 false
 @const.int = constant i32 zeroinitializer
 ; CHECK: @const.int = constant i32 0
 @const.float = constant double 0.0
 ; CHECK: @const.float = constant double 0.0
 @const.null = constant i8* null
 ; CHECK: @const.null = constant i8* null
[e-mail] = constant i32 vscale
+; CHECK: @const.vscale = constant i32 vscale
 %const.struct.type = type { i32, i8 }
 %const.struct.type.packed = type <{ i32, i8 }>
 @const.struct = constant %const.struct.type { i32 -1, i8 undef }
 ; CHECK: @const.struct = constant %const.struct.type { i32 -1, i8 undef }
 @const.struct.packed = constant %const.struct.type.packed <{ i32 -1, i8 1 }>
 ; CHECK: @const.struct.packed = constant %const.struct.type.packed <{ i32 -1, i8 1 }>
 
 ; CHECK: @constant.array.i8  = constant [3 x i8] c"\00\01\00"