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

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:259
   mutable TypeInfoMap MemoizedTypeInfo;
+  /// /// A cache from types to size and preferred alignment information.
+  mutable TypeInfoMap MemoizedPreferredTypeInfo;
----------------
nit: Extra /// at the beginning of the comment could be removed.


================
Comment at: clang/include/clang/AST/ASTContext.h:2133
+  /// 'arm_sve_vector_bits'. Should only be called if T->isVLST().
+  unsigned getBitwidthForAttributedSveType(const Type *T) const;
+
----------------
Maybe you want to move up to a newer base? I don't see how this change belong to this patch.


================
Comment at: clang/lib/AST/ASTContext.cpp:1872
 
-  // This call can invalidate MemoizedTypeInfo[T], so we need a second lookup.
-  TypeInfo TI = getTypeInfoImpl(T);
-  MemoizedTypeInfo[T] = TI;
-  return TI;
+    // This call can invalidate MemoizedTypeInfo[T], so we need a second lookup.
+    TypeInfo TI = getTypeInfoImpl(T, NeedsPreferredAlignment);
----------------
nit: This comment needs an update, as it could invalidate MemoizedPreferredTypeInfo depending on the flag.


================
Comment at: clang/lib/AST/ASTContext.cpp:2106
       Width = Target->getDoubleWidth();
-      Align = Target->getDoubleAlign();
+      setAlign(Width, Target->getDoubleAlign());
       break;
----------------
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.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4539
     CharUnits CCAlign = getParamTypeAlignment(Ty);
     CharUnits TyAlign = getContext().getTypeAlignInChars(Ty);
 
----------------
Question: 
It looks like getNaturalAlignIndirect and getTypeAlignInChars here are all returning ABI alignment.
But according to the comments, we should use a preferred alignment when it's a complete object. Isn't this complete object? Or I'm missing something?


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

https://reviews.llvm.org/D86790