[PATCH] D86790: [FE] Use preferred alignment instead of ABI alignment for complete object when applicable

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:256
 
-  /// A cache from types to size and alignment information.
   using TypeInfoMap = llvm::DenseMap<const Type *, struct TypeInfo>;
+  /// A cache from types to size and ABI-specified alignment information.
----------------
Please undo all the changes to the ASTContext API (both in this file and in ASTContext.cpp) which add the boolean NeedsPreferredAlignment to getTypeInfo/getTypeAlign and related functions; it's not necessary. (OK to leave the change to getAlignmentIfKnown).


================
Comment at: clang/include/clang/AST/ASTContext.h:2165
+  /// from alignment attributes).
+  unsigned getTypeAlignIfKnown(QualType T,
+                               bool NeedsPreferredAlignment = false) const;
----------------
The change to this function is OK. The wording of the comment change is confusing, though.

How-about:
Return the alignment of a type, in bits, or 0 if the type is incomplete and we cannot determine the alignment (for example, from alignment attributes). The returned alignment is the Preferred alignment if UsePreferredAlignment is true, otherwise is the ABI alignment.


================
Comment at: clang/lib/AST/ASTContext.cpp:1850
   if (!T->isIncompleteType())
-    return getTypeAlign(T);
+    return getTypeAlign(T, NeedsPreferredAlignment);
 
----------------
return NeedsPreferredAlignment ? getPreferredTypeAlign(T) : getTypeAlign(T);


================
Comment at: clang/lib/AST/ASTContext.cpp:2106
       Width = Target->getDoubleWidth();
-      Align = Target->getDoubleAlign();
+      setAlign(Width, Target->getDoubleAlign());
       break;
----------------
jasonliu wrote:
> I have two concerns here:
> 1. I feel like we are having duplicate logic between here and ASTContext::getPreferredTypeAlign for how to get preferred alignment. 
> 2.  These logic are too AIX-centric and only modified the places for where types are affected by preferred alignment on AIX. What if on some platforms, BuiltinType::Float have different preferred alignments? Or Width is not the preferred alignment on certain platform for double/long double.
This becomes moot, if my comments are resolved.


================
Comment at: clang/lib/AST/ASTContext.cpp:2501
   uint64_t TypeSize = getTypeSize(T.getTypePtr());
-  return std::max(getTypeAlign(T), getTargetInfo().getMinGlobalAlign(TypeSize));
+  return std::max(getTypeAlign(T, true /* NeedsPreferredAlignment */),
+                  getTargetInfo().getMinGlobalAlign(TypeSize));
----------------
Can just call getPreferredTypeAlign here. (Add a QualType overload to getPreferredTypeAlign).


================
Comment at: clang/lib/CodeGen/CGAtomic.cpp:814
+  std::tie(sizeChars, alignChars) = getContext().getTypeInfoInChars(
+      AtomicTy, true /* NeedsPreferredAlignment */);
   uint64_t Size = sizeChars.getQuantity();
----------------
Xiangling_L wrote:
> jyknight wrote:
> > This is wrong.
> Can you explain a bit why it's wrong?
It's not allocating new memory, it's accessing a pointer to memory somewhere else, so if alignChars was used, this change would be actively wrong -- using the wrong value.

However, alignChars is unused, so it's wrong only in a way that ultimately makes no difference.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:1573
                         allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getTypeAlignInChars(allocType);
+  CharUnits allocAlign = getContext().getTypeAlignInChars(
+      allocType, true /* NeedsPreferredAlignment */);
----------------
I'd add the 1-line getPreferredTypeAlignInChars function, and call it.


================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:1826
+        getContext().toCharUnitsFromBits(getContext().getTypeAlignIfKnown(
+            DeleteTy, true /* NeedsPreferredAlignment */));
     llvm::Value *Align = llvm::ConstantInt::get(ConvertType(AlignValType),
----------------
The "getTypeAlignIfKnown" function is so special-purpose, it's probably fine to have this 1 function keep the "Preferred" boolean. This can stay.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2114
   return std::max(CharUnits::fromQuantity(CGM.SizeSizeInBytes),
-                  CGM.getContext().getTypeAlignInChars(elementType));
+                  CGM.getContext().getTypeAlignInChars(
+                      elementType, true /* NeedsPreferredAlignment */));
----------------
getPreferredTypeAlignInChars.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2133
+      SizeSize,
+      Ctx.getTypeAlignInChars(ElementType, true /* NeedsPreferredAlignment */));
   assert(CookieSize == getArrayCookieSizeImpl(ElementType));
----------------
getPreferredTypeAlignInChars.


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

https://reviews.llvm.org/D86790