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

[Renato Golin via Phabricator via llvm-commits]

rengolin added reviewers: chandlerc, echristo, lattner, majnemer.
rengolin added a comment.

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



================
Comment at: include/llvm-c/Core.h:1188
       macro(UndefValue)                     \
+      macro(VScaleValue)                    \
     macro(Instruction)                      \
----------------
huntergr wrote:
> rengolin wrote:
> > Not being in the same order as above may confuse people looking for it?
> 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.
Makes sense.


================
Comment at: include/llvm/IR/Value.def:92
 HANDLE_CONSTANT_MARKER(ConstantFirstVal, Function)
-HANDLE_CONSTANT_MARKER(ConstantLastVal, ConstantTokenNone)
+HANDLE_CONSTANT_MARKER(ConstantLastVal, VScaleValue)
 HANDLE_CONSTANT_MARKER(ConstantDataFirstVal, UndefValue)
----------------
huntergr wrote:
> rengolin wrote:
> > I don't get this change...
> 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.
Right, I see.


================
Comment at: lib/IR/Constants.cpp:812
+  // Free the constant and any dangling references to it.
+  getContext().pImpl->VSVConstants.erase(getType());
+}
----------------
aemerson wrote:
> rengolin wrote:
> > 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.
> Yes, in the same way you can have i32 undef, i64 undef etc.
Right, makes sense.


https://reviews.llvm.org/D32737