WIP: Disable optimization in emitStoresForConstant

[vitalybuka]

Summary

Not rely a review, but removing this code I see slight improvements in
binary size on CTMark. Looks like with "ptr" patches MemCpyOptPass is
capable to handle this.

WDYT?

[jfb]

That's interesting. If MemCpyOpt has picked up the slack and clang can be simpler here, then let's remove clang's "intelligence". I do want to make sure that it handles all the cases this handles though, and does indeed generate great code on both x86-64 and ARM64. I think we need to create a list of test cases and make sure MemCpyOpt covers them. Further, I'd also like to check that performance is also still the same (not just code size).

I would still keep scalar stores (as you did above), and maybe update canDoSingleStore to also handle stores to a struct that only has a single element.

[vitalybuka]

Remove single store

[jfb]

I would keep single stores because it's pretty silly codegen to copy from a global for that.

I would also verify that -O0 isn't affected too badly by this change.

[vitalybuka]

In D64675#1584016, @jfb wrote:

I would keep single stores because it's pretty silly codegen to copy from a global for that.

I would also verify that -O0 isn't affected too badly by this change.

I will. And maybe I'll measure compilation time as well.

So far I tried -O3 and -Os.
Also I tried Chromium arm, arm64, x86_64. No regression in binary size without autoinit, with zero or pattern init.

clang/lib/CodeGen/CGDecl.cpp

bool canDoSingleStore = Ty->isIntOrIntVectorTy() ||
                           Ty->isPtrOrPtrVectorTy() || Ty->isFPOrFPVectorTy();
   if (canDoSingleStore) {
     Builder.CreateStore(constant, Loc, isVolatile);
     return;
   }
 
   auto *SizeVal = llvm::ConstantInt::get(CGM.IntPtrTy, ConstantSize);
 
-  // If the initializer is all or mostly the same, codegen with bzero / memset
-  // then do a few stores afterward.
-  if (shouldUseBZeroPlusStoresToInitialize(constant, ConstantSize)) {
-    Builder.CreateMemSet(Loc, llvm::ConstantInt::get(CGM.Int8Ty, 0), SizeVal,
-                         isVolatile);
-
-    bool valueAlreadyCorrect =
-        constant->isNullValue() || isa<llvm::UndefValue>(constant);
-    if (!valueAlreadyCorrect) {
-      Loc = Builder.CreateBitCast(Loc, Ty->getPointerTo(Loc.getAddressSpace()));
-      emitStoresForInitAfterBZero(CGM, constant, Loc, isVolatile, Builder);
-    }
-    return;
-  }
-
-  // If the initializer is a repeated byte pattern, use memset.
-  llvm::Value *Pattern =
-      shouldUseMemSetToInitialize(constant, ConstantSize, CGM.getDataLayout());
-  if (Pattern) {
-    uint64_t Value = 0x00;
-    if (!isa<llvm::UndefValue>(Pattern)) {
-      const llvm::APInt &AP = cast<llvm::ConstantInt>(Pattern)->getValue();
-      assert(AP.getBitWidth() <= 8);
-      Value = AP.getLimitedValue();
-    }
-    Builder.CreateMemSet(Loc, llvm::ConstantInt::get(CGM.Int8Ty, Value), SizeVal,
-                         isVolatile);
-    return;
-  }
+  // // If the initializer is all or mostly the same, codegen with bzero / memset
+  // // then do a few stores afterward.
+  // if (shouldUseBZeroPlusStoresToInitialize(constant, ConstantSize)) {
+  //   Builder.CreateMemSet(Loc, llvm::ConstantInt::get(CGM.Int8Ty, 0), SizeVal,
+  //                        isVolatile);
+
+  //   bool valueAlreadyCorrect =
+  //       constant->isNullValue() || isa<llvm::UndefValue>(constant);
+  //   if (!valueAlreadyCorrect) {
+  //     Loc = Builder.CreateBitCast(Loc, Ty->getPointerTo(Loc.getAddressSpace()));
+  //     emitStoresForInitAfterBZero(CGM, constant, Loc, isVolatile, Builder);
+  //   }
+  //   return;
+  // }
+
+  // // If the initializer is a repeated byte pattern, use memset.
+  // llvm::Value *Pattern =
+  //     shouldUseMemSetToInitialize(constant, ConstantSize, CGM.getDataLayout());
+  // if (Pattern) {
+  //   uint64_t Value = 0x00;
+  //   if (!isa<llvm::UndefValue>(Pattern)) {
+  //     const llvm::APInt &AP = cast<llvm::ConstantInt>(Pattern)->getValue();
+  //     assert(AP.getBitWidth() <= 8);
+  //     Value = AP.getLimitedValue();
+  //   }
+  //   Builder.CreateMemSet(Loc, llvm::ConstantInt::get(CGM.Int8Ty, Value), SizeVal,
+  //                        isVolatile);
+  //   return;
+  // }
 
   // If the initializer is small, use a handful of stores.
-  if (shouldSplitConstantStore(CGM, ConstantSize)) {
-    if (auto *STy = dyn_cast<llvm::StructType>(Ty)) {
-      // FIXME: handle the case when STy != Loc.getElementType().
-      if (STy == Loc.getElementType()) {
-        for (unsigned i = 0; i != constant->getNumOperands(); i++) {
-          Address EltPtr = Builder.CreateStructGEP(Loc, i);
-          emitStoresForConstant(
-              CGM, D, EltPtr, isVolatile, Builder,
-              cast<llvm::Constant>(Builder.CreateExtractValue(constant, i)));
-        }
-        return;
-      }
-    } else if (auto *ATy = dyn_cast<llvm::ArrayType>(Ty)) {
-      // FIXME: handle the case when ATy != Loc.getElementType().
-      if (ATy == Loc.getElementType()) {
-        for (unsigned i = 0; i != ATy->getNumElements(); i++) {
-          Address EltPtr = Builder.CreateConstArrayGEP(Loc, i);
-          emitStoresForConstant(
-              CGM, D, EltPtr, isVolatile, Builder,
-              cast<llvm::Constant>(Builder.CreateExtractValue(constant, i)));
-        }
-        return;
-      }
-    }
-  }
+  // if (shouldSplitConstantStore(CGM, ConstantSize)) {
+  //   if (auto *STy = dyn_cast<llvm::StructType>(Ty)) {
+  //     // FIXME: handle the case when STy != Loc.getElementType().
+  //     if (STy == Loc.getElementType()) {
+  //       for (unsigned i = 0; i != constant->getNumOperands(); i++) {
+  //         Address EltPtr = Builder.CreateStructGEP(Loc, i);
+  //         emitStoresForConstant(
+  //             CGM, D, EltPtr, isVolatile, Builder,
+  //             cast<llvm::Constant>(Builder.CreateExtractValue(constant, i)));
+  //       }
+  //       return;
+  //     }
+  //   } else if (auto *ATy = dyn_cast<llvm::ArrayType>(Ty)) {
+  //     // FIXME: handle the case when ATy != Loc.getElementType().
+  //     if (ATy == Loc.getElementType()) {
+  //       for (unsigned i = 0; i != ATy->getNumElements(); i++) {
+  //         Address EltPtr = Builder.CreateConstArrayGEP(Loc, i);
+  //         emitStoresForConstant(
+  //             CGM, D, EltPtr, isVolatile, Builder,
+  //             cast<llvm::Constant>(Builder.CreateExtractValue(constant, i)));
+  //       }
+  //       return;
+  //     }
+  //   }
+  // }
 
   // Copy from a global.
   Builder.CreateMemCpy(Loc,
                        createUnnamedGlobalForMemcpyFrom(
                            CGM, D, Builder, constant, Loc.getAlignment()),
                        SizeVal, isVolatile);
 }