[CGExpr] Use getCharWidth() more consistently in CCGExprConstant. NFC

[bjope]

Summary

Most of CGExprConstant.cpp is using the CharUnits abstraction
and is using getCharWidth() (directly of indirectly) when converting
between size of a char and size in bits. This patch is making that
abstraction more consistent by adding CharTy to the CodeGenTypeCache
(honoring getCharWidth() when mapping from char to LLVM IR types,
instead of using Int8Ty directly).

[lebedev.ri]

As far as i recall, all RFC's about LLVM support for non-8-bit char targets didn't end in favor of the proposal
I'd say that until there's general consensus/buy-in (including testing path), cleaning stuff up will only confuse things.
CC @jfb

[bjope]

In D94979#2507242, @lebedev.ri wrote:

As far as i recall, all RFC's about LLVM support for non-8-bit char targets didn't end in favor of the proposal
I'd say that until there's general consensus/buy-in (including testing path), cleaning stuff up will only confuse things.
CC @jfb

Well, I haven't even added reviewers. But if it hurts so much maybe I should abandon it ;-)

But from my point of view this patch isn't really adding non-8-bit char support. But there is an abstraction in clang (such as ASTContext) with getCharWidth(), toCharUnitsFromBits() etc. And it is confusing when things don't add up (such as calculating the number of chars using toCharUnitsFromBits(), indirectly using getCharWidth(), and then later ignoring it and assuming that the char width is 8. I don't see how being more consistent is more confusing.

[rjmccall]

I don't mind generally abstracting Clang to handle this better.

clang/lib/CodeGen/CodeGenModule.h

1229 ↗

(On Diff #317603)

Please just add a CharTy to CodeGenTypeCache.

[bjope]

Updated to add CharTy to the CodeGenTypeCache (based on review feedback).

[bjope]

@lebedev.ri : I think I need your blessing as well on this, considering your earlier concerns. Is it still just confusing? (I doubt that we want/can replace all uses of getCharWidth/getIntWidth etc in clang with integers)

[lebedev.ri]

I don't particularly want/like to straight-up derail this,
but my *personal* opinion still hasn't changed, and it is not unsubstantiated,
see https://lists.llvm.org/pipermail/llvm-dev/2019-May/132080.html,
https://lists.llvm.org/pipermail/llvm-dev/2019-October/136115.html, etc,

I think the fact that last *two* recent-ish RFC's on this didn't succeed
suggests that a third one might be in order, perhaps with better arguments
this time, or maybe the previous opponents will just not block this..

TBN, i don't dislike the feature per-se, at least without seeing the full impact of the cleanup.

[rjmccall]

Clang has generally made a point of trying to be as neutral about CHAR_BITS as it can be, even though LLVM has not. As the code owner of this code, I view this as a fix consistent with that, and I consider it acceptable to land.

Of course, if you are pursuing these patches out of an expectation that you'll eventually get Clang to support compiling targets with non-8-bit-addressing, you need to understand that (at least for most cases) that depends on LLVM supporting such targets, and so far the LLVM community has been pretty strongly opposed to that idea. But Clang has *internally* already accepted the abstraction costs of CharUnits, and we should to be as consistent as possible about that in our own code.

clang/lib/CodeGen/CGExprConstant.cpp

CharUnits getSize(llvm::Type *Ty) const {
     return CharUnits::fromQuantity(CGM.getDataLayout().getTypeAllocSize(Ty));
   }
 
   CharUnits getSize(const llvm::Constant *C) const {
     return getSize(C->getType());
   }
 
   llvm::Constant *getPadding(CharUnits PadSize) const {
-    llvm::Type *Ty = CGM.Int8Ty;
+    llvm::Type *Ty = CGM.CharTy;
     if (PadSize > CharUnits::One())
       Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
     return llvm::UndefValue::get(Ty);
   }
 
   llvm::Constant *getZeroes(CharUnits ZeroSize) const {
-    llvm::Type *Ty = llvm::ArrayType::get(CGM.Int8Ty, ZeroSize.getQuantity());
+    llvm::Type *Ty = llvm::ArrayType::get(CGM.CharTy, ZeroSize.getQuantity());
     return llvm::ConstantAggregateZero::get(Ty);
   }
 };
 
 /// Incremental builder for an llvm::Constant* holding a struct or array
 /// constant.
 class ConstantAggregateBuilder : private ConstantAggregateBuilderUtils {
   /// The elements of the constant. These two arrays must have the same size;
case CK_ToUnion: {
       SmallVector<llvm::Type*, 2> Types;
       Elts.push_back(C);
       Types.push_back(C->getType());
       unsigned CurSize = CGM.getDataLayout().getTypeAllocSize(C->getType());
       unsigned TotalSize = CGM.getDataLayout().getTypeAllocSize(destTy);
 
       assert(CurSize <= TotalSize && "Union size mismatch!");
       if (unsigned NumPadBytes = TotalSize - CurSize) {
-        llvm::Type *Ty = CGM.Int8Ty;
+        llvm::Type *Ty = CGM.CharTy;
         if (NumPadBytes > 1)
           Ty = llvm::ArrayType::get(Ty, NumPadBytes);
 
         Elts.push_back(llvm::UndefValue::get(Ty));
         Types.push_back(Ty);
       }
 
       llvm::StructType *STy = llvm::StructType::get(VMContext, Types, false);

clang/lib/CodeGen/CodeGenModule.cpp

CodeGenModule::CodeGenModule(ASTContext &C, const HeaderSearchOptions &HSO,
   DoubleTy = llvm::Type::getDoubleTy(LLVMContext);
   PointerWidthInBits = C.getTargetInfo().getPointerWidth(0);
   PointerAlignInBytes =
     C.toCharUnitsFromBits(C.getTargetInfo().getPointerAlign(0)).getQuantity();
   SizeSizeInBytes =
     C.toCharUnitsFromBits(C.getTargetInfo().getMaxPointerWidth()).getQuantity();
   IntAlignInBytes =
     C.toCharUnitsFromBits(C.getTargetInfo().getIntAlign()).getQuantity();
+  CharTy =
+    llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getCharWidth());
   IntTy = llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getIntWidth());
   IntPtrTy = llvm::IntegerType::get(LLVMContext,
     C.getTargetInfo().getMaxPointerWidth());
   Int8PtrTy = Int8Ty->getPointerTo(0);
   Int8PtrPtrTy = Int8PtrTy->getPointerTo(0);
   AllocaInt8PtrTy = Int8Ty->getPointerTo(
       M.getDataLayout().getAllocaAddrSpace());
   ASTAllocaAddressSpace = getTargetCodeGenInfo().getASTAllocaAddressSpace();

clang/lib/CodeGen/CodeGenTypeCache.h

struct CodeGenTypeCache {
   /// i8, i16, i32, and i64
   llvm::IntegerType *Int8Ty, *Int16Ty, *Int32Ty, *Int64Ty;
   /// half, bfloat, float, double
   llvm::Type *HalfTy, *BFloatTy, *FloatTy, *DoubleTy;
 
   /// int
   llvm::IntegerType *IntTy;
 
+  /// char
+  llvm::IntegerType *CharTy;
+
   /// intptr_t, size_t, and ptrdiff_t, which we assume are the same size.
   union {
     llvm::IntegerType *IntPtrTy;
     llvm::IntegerType *SizeTy;
     llvm::IntegerType *PtrDiffTy;
   };
 
   /// void* in address space 0