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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
