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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenTypes.h:138
+  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false,
+                                bool EnforceFixedLengthSVEAttribute = false);
 
----------------
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.


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

https://reviews.llvm.org/D83553