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