[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked an inline comment as done.
c-rhodes added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenTypes.h:138
+  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false,
+                                bool EnforceFixedLengthSVEAttribute = false);
 
----------------
efriedma wrote:
> c-rhodes wrote:
> > efriedma wrote:
> > > The default for EnforceFixedLengthSVEAttribute seems backwards; I would expect that almost everywhere that calls ConvertTypeForMem actually wants the fixed-length type.  The scalable type only exists in registers.
> > > The default for EnforceFixedLengthSVEAttribute seems backwards; I would expect that almost everywhere that calls ConvertTypeForMem actually wants the fixed-length type. The scalable type only exists in registers.
> > 
> > It has no effect unless `T->isVLST()` so I think it makes sense.
> My question is "why is the current default for EnforceFixedLengthSVEAttribute correct?" You answer for that is "because VLST types are rare"?  I'm not sure how that's related.
> 
> Essentially, the issue is that ConvertTypeForMem means "I'm allocating something in memory; what is its type?".  Except for a few places where we've specifically added handling to make it work, the code assumes scalable types don't exist.  So in most places, we want the fixed version.  With the current default, I'm afraid we're going to end up with weird failures with various constructs you haven't tested.
> 
> I guess if there's some large number of places where the current default is actually beneficial, the current patch wouldn't make it obvious, but my intuition is that are few places like that.
>> My question is "why is the current default for EnforceFixedLengthSVEAttribute correct?" You answer for that is "because VLST types are rare"? I'm not sure how that's related.

>Essentially, the issue is that ConvertTypeForMem means "I'm allocating something in memory; what is its type?". Except for a few places where we've specifically added handling to make it work, the code assumes scalable types don't exist. So in most places, we want the fixed version. With the current default, I'm afraid we're going to end up with weird failures with various constructs you haven't tested.

Sorry I misunderstood what you meant. I think you're right that does make sense, I guess the benefit of defaulting to false is (hopefully) those failures would have come to our attention and we could explicitly add test cases for those, although I suspect the same applies with your suggestion with the added benefit of us supporting constructs we haven't explicitly tested as you say. Anyhow, I've made the change, cheers!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83553/new/

https://reviews.llvm.org/D83553