[cfe-dev] Fw:about element size/alignment in an array

[林政宗 via cfe-dev]

Thanks, Tim.


Actually, I am implementing a new backend. The hardware support vector of 512 bits. But vector type should be aligned to 1024 bits in memory. So I added "v512:1024" to datalayout for both clang and llvm. 
Unfortunately Clang will crash for some cases. The reason is that, when calculating the size of an array of vector, llvm will consider the alignment of the element of an array, but clang will not. When calculating the size of type [10 x <16 x i32>], llvm considers each element's store size which aligns each element's size to its alignment.  
After adding "v512:1024" to datalayout, llvm considers the size of [10 x <16 x i32>] to be 10240 bits.
But Clang still considers its size to be 5120 bits. So when this array is in a struct, Clang crashes in CGRecordLowering::clipTailPadding().
--------------------------------------------------------------------------------------------------------------------------
/// getConstantArrayInfoInChars - Performing the computation in CharUnits
/// instead of in bits prevents overflowing the uint64_t for some large arrays.
std::pair<CharUnits, CharUnits>
static getConstantArrayInfoInChars(const ASTContext &Context,
                                   const ConstantArrayType *CAT) {
  std::pair<CharUnits, CharUnits> EltInfo =
      Context.getTypeInfoInChars(CAT->getElementType());
  uint64_t Size = CAT->getSize().getZExtValue();
  assert((Size == 0 || static_cast<uint64_t>(EltInfo.first.getQuantity()) <=
              (uint64_t)(-1)/Size) &&
         "Overflow in array type char size evaluation");
  uint64_t Width = EltInfo.first.getQuantity() * Size;                  // I make modification here
  unsigned Align = EltInfo.second.getQuantity();
  if (!Context.getTargetInfo().getCXXABI().isMicrosoft() ||
      Context.getTargetInfo().getPointerWidth(0) == 64)
    Width = llvm::alignTo(Width, Align);
  return std::make_pair(CharUnits::fromQuantity(Width),
                        CharUnits::fromQuantity(Align));
}
--------------------------------------------------------------------------------------------------------------------------
So I make modification where Width is calculated. 
--------------------------------------------------------------------------------------------------------------------------
uint64_t Width = EltInfo.first.getQuantity() * Size;                               // before modification
uint64_t Width = llvm::alignTo(EltInfo.first.getQuantity(), EltInfo.second.getQuantity())* Size; // after modification
----------------------------------------------------------------------------------------------------------------------------
But I am worried that my modification would make test cases for other target fail.
How could I know if my modification would make tests for other target like x86/Arm fail? 
How could I test it? I have read the reference,LLVM Testing Infrastructure Guide — LLVM 12 documentation.
But I don't have a clear idea.


Best Regards,
Jerry










-------- Forwarding messages --------
From: "林政宗" <jackie_linzz at 126.com>
Date: 2020-12-08 20:15:31
To: "llvm-dev at lists.llvm.org" <llvm-dev at lists.llvm.org>,"cfe-dev at lists.llvm.org" <cfe-dev at lists.llvm.org>
Subject: about element size/alignment in an array

Hi, there.


I met a problem about element size/alignment in an array. I defined the following vector type.
-------------------------------------------------------------------------------------------------------------------------
typedef int __attribute__((__vector_size__(64), aligned(128))) v16i32;
struct container {
    v16i32 arr[10];
    v16i32 val;
} stu;
-------------------------------------------------------------------------------------------------------------------------
Its size is 512bits. And alignment is 1024bits.
I also defined the datalayout in clang and llvm, "v512:1024". I need vector of 512bits to be aligned to 1024bits no matter whether it is in an array or not. 


but I saw the code as below in clang/lib/AST/ASTContext.cpp. line 1749
--------------------------------------------------------------------------------------------------------------------------
/// getConstantArrayInfoInChars - Performing the computation in CharUnits
/// instead of in bits prevents overflowing the uint64_t for some large arrays.
std::pair<CharUnits, CharUnits>
static getConstantArrayInfoInChars(const ASTContext &Context,
                                   const ConstantArrayType *CAT) {
  std::pair<CharUnits, CharUnits> EltInfo =
      Context.getTypeInfoInChars(CAT->getElementType());
  uint64_t Size = CAT->getSize().getZExtValue();
  assert((Size == 0 || static_cast<uint64_t>(EltInfo.first.getQuantity()) <=
              (uint64_t)(-1)/Size) &&
         "Overflow in array type char size evaluation");
  uint64_t Width = EltInfo.first.getQuantity() * Size;                  //here
  unsigned Align = EltInfo.second.getQuantity();
  if (!Context.getTargetInfo().getCXXABI().isMicrosoft() ||
      Context.getTargetInfo().getPointerWidth(0) == 64)
    Width = llvm::alignTo(Width, Align);
  return std::make_pair(CharUnits::fromQuantity(Width),
                        CharUnits::fromQuantity(Align));
}
--------------------------------------------------------------------------------------------------------------------------
when caculating the size of an array, the alignment of element is not considered. Thus the width of the array "arr" in struct container is 512x10bits, not 1024x10bits.
And the offset of "val" in struct container is 512x10bits. That's 640bytes.
but it is not the way in which size of "arr" is caculated in CGRecordLowering::clipTailPadding(), line 586 of clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
--------------------------------------------------------------------------------------------------------------------------
void CGRecordLowering::clipTailPadding() {
  std::vector<MemberInfo>::iterator Prior = Members.begin();
  CharUnits Tail = getSize(Prior->Data);   //here
  for (std::vector<MemberInfo>::iterator Member = Prior + 1,
                                         MemberEnd = Members.end();
       Member != MemberEnd; ++Member) {
    // Only members with data and the scissor can cut into tail padding.
    if (!Member->Data && Member->Kind != MemberInfo::Scissor)
      continue;
    if (Member->Offset < Tail) {               // and here
      assert(Prior->Kind == MemberInfo::Field &&
             "Only storage fields have tail padding!");
      if (!Prior->FD || Prior->FD->isBitField())
        Prior->Data = getByteArrayType(bitsToCharUnits(llvm::alignTo(
            cast<llvm::IntegerType>(Prior->Data)->getIntegerBitWidth(), 8)));
      else {
        assert(Prior->FD->hasAttr<NoUniqueAddressAttr>() &&
               "should not have reused this field's tail padding");     // doesn't meet this assert
        Prior->Data = getByteArrayType(
            Context.getTypeInfoDataSizeInChars(Prior->FD->getType()).first);
      }
    }
    if (Member->Data)
      Prior = Member;
    Tail = Prior->Offset + getSize(Prior->Data);
  }
}
--------------------------------------------------------------------------------------------------------------------------
the "getSize(Prior->Data)" function will consider the alignment of element. When "Prior" points to the first field "arr" of struct container, "Tail" is 1024x10/8 = 1280 bytes. That's the size of type [10 x <16 x i32>] considering alignment.
At the first iteration of for loop, "Member" points to the second field "val" of struct container. "Member->Offset" is 640bytes.
"if (Member->Offset < Tail)" is true, and the execution goes in. And later "else" branch is executed. 
The "assert" in else clause will fail, and clang will crash.


I think when caculating the size of an array, the alignment of element should also be considered. But it is not the case in code.
I am not sure whether it is a bug in clang or there is something I don't know.
Could anyone help me? Thanks in advance!




Best Regards,
Jerry










 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20201217/e2fd96ce/attachment-0001.html>