[CodeGen] Fix assertion failure on large types store

[kovdan01]

Summary

The following IR:

%struct.large = type { [2500000 x i8] }

define void @foo(%struct.large %s, %struct.large* %p) {
  store %struct.large %s, %struct.large* %p, align 1
  ret void
}

caused assertion 'NumBits <= MAX_INT_BITS && "bitwidth too large"' failure.

The IR above is a minimized version from IR from an actual bug report. Keeping
in mind that such IR is discouraged, the compiler should not fail on a formally
valid input. The patch fixes this issue.

Fixing that also requires widening NumValues and NumOperands fields of SDNode
to avoid fail of another assertion about insufficient bit width of those fields.
Reordering of SDNode fields was done to keep sizeof(SDNode) == 80 on x86_64
Linux builds.

[kovdan01]

UPD: fixed the problem, keep the comment for history.

As for now, the patch contains no tests. The reason is that a minimal test lasts unacceptably long (didn't manage to wait until end) with assertions enabled - the bottleneck is checkForCyclesHelper function. With expensive checks enabled, one more bottleneck appears: DAGTypeLegalizer::PerformExpensiveChecks. When commenting those functions bodies, minimal test with release-checked compiler lasts for about 160s on my PC. Could anyone suggest a way to test this patch correctly? IMHO looks like we can leave it without special test because the fix is pretty trivial, while I personally see no obvious way to test it.--

[kovdan01]

Would be glad if someone could review this. Thanks!

[RKSimon]

llvm/lib/CodeGen/CodeGenPrepare.cpp

7463

Isn't this hard-coded to unsigned/uint32_t? What about exposing a typedef in SDNode instead so we can use std::numeric_limits? .

[craig.topper]

llvm/lib/CodeGen/CodeGenPrepare.cpp

7463

MAX_INT_BITS is 1<<23. It's related to there being 24 bits of subclass data available in llvm::Type

[craig.topper]

In D124085#3461526, @kovdan01 wrote:

As for now, the patch contains no tests. The reason is that a minimal test lasts unacceptably long (didn't manage to wait until end) with assertions enabled - the bottleneck is checkForCyclesHelper function. With expensive checks enabled, one more bottleneck appears: DAGTypeLegalizer::PerformExpensiveChecks. When commenting those functions bodies, minimal test with release-checked compiler lasts for about 160s on my PC. Could anyone suggest a way to test this patch correctly? IMHO looks like we can leave it without special test because the fix is pretty trivial, while I personally see no obvious way to test it.

Since this in CodeGenPrepare, couldn't you write a test that uses opt -codegenprepare and not go through SelectionDAG?

[kovdan01]

In D124085#3470201, @craig.topper wrote:

In D124085#3461526, @kovdan01 wrote:

As for now, the patch contains no tests. The reason is that a minimal test lasts unacceptably long (didn't manage to wait until end) with assertions enabled - the bottleneck is checkForCyclesHelper function. With expensive checks enabled, one more bottleneck appears: DAGTypeLegalizer::PerformExpensiveChecks. When commenting those functions bodies, minimal test with release-checked compiler lasts for about 160s on my PC. Could anyone suggest a way to test this patch correctly? IMHO looks like we can leave it without special test because the fix is pretty trivial, while I personally see no obvious way to test it.

Since this in CodeGenPrepare, couldn't you write a test that uses opt -codegenprepare and not go through SelectionDAG?

Thanks for useful advice! That works, added llvm/test/CodeGen/Generic/codegen_prepare_large_structs.ll.

[craig.topper]

llvm/test/CodeGen/Generic/codegen_prepare_large_structs.ll

1 ↗

(On Diff #424805)

I don't think tests in the Generic directory can use a triple. It requires the target to be built, but a target can be disabled by passing LLVM_TARGETS_TO_BUILD to cmake.

[kovdan01]

llvm/test/CodeGen/Generic/codegen_prepare_large_structs.ll

1 ↗

(On Diff #424805)

Makes sense, fixed, thanks!

[craig.topper]

Please split the CodeGenPrepare and SelectionDAG changes into separate patches.

llvm/include/llvm/CodeGen/SelectionDAGNodes.h

private:
   /// The types of the values this node defines.  SDNode's may
   /// define multiple values simultaneously.
   const EVT *ValueList;
 
   /// List of uses for this SDNode.
   SDUse *UseList = nullptr;
 
   /// The number of entries in the Operand/Value list.
-  unsigned short NumOperands = 0;
-  unsigned short NumValues;
+  unsigned NumOperands = 0;
+  unsigned NumValues;
+
+  /// Source line information.
+  DebugLoc debugLoc;
 
   // The ordering of the SDNodes. It roughly corresponds to the ordering of the
   // original LLVM instructions.
   // This is used for turning off scheduling, because we'll forgo
   // the normal scheduling algorithms and output the instructions according to
   // this ordering.
   unsigned IROrder;
 
-  /// Source line information.
-  DebugLoc debugLoc;
-
   /// Return a pointer to the specified value type.
   static const EVT *getValueTypeList(EVT VT);
 
   SDNodeFlags Flags;
 
 public:
   /// Unique and persistent id per SDNode in the DAG. Used for debug printing.
   /// We do not place that under `#if LLVM_ENABLE_ABI_BREAKING_CHECKS`
protected:
   }
 
   /// Create an SDNode.
   ///
   /// SDNodes are created without any operands, and never own the operand
   /// storage. To add operands, see SelectionDAG::createOperands.
   SDNode(unsigned Opc, unsigned Order, DebugLoc dl, SDVTList VTs)
       : NodeType(Opc), ValueList(VTs.VTs), NumValues(VTs.NumVTs),
-        IROrder(Order), debugLoc(std::move(dl)) {
+        debugLoc(std::move(dl)), IROrder(Order) {
     memset(&RawSDNodeBits, 0, sizeof(RawSDNodeBits));
     assert(debugLoc.hasTrivialDestructor() && "Expected trivial destructor");
-    assert(NumValues == VTs.NumVTs &&
-           "NumValues wasn't wide enough for its operands!");
   }
 
   /// Release the operands and set this node to have zero operands.
   void DropOperands();
 };
 
 /// Wrapper class for IR location info (IR ordering and DebugLoc) to be passed
 /// into SDNode creation functions.

llvm/lib/CodeGen/CodeGenPrepare.cpp

static bool splitMergedValStore(StoreInst &SI, const DataLayout &DL,
   if (isa<ScalableVectorType>(StoreType))
     return false;
 
   if (!DL.typeSizeEqualsStoreSize(StoreType) ||
       DL.getTypeSizeInBits(StoreType) == 0)
     return false;
 
   unsigned HalfValBitSize = DL.getTypeSizeInBits(StoreType) / 2;
+  // Cannot split store into two integer stores if initial store type
+  // is wider than 2 * MAX_INT_BITS
+  if (HalfValBitSize > IntegerType::MAX_INT_BITS)
+    return false;
+
   Type *SplitStoreType = Type::getIntNTy(SI.getContext(), HalfValBitSize);
   if (!DL.typeSizeEqualsStoreSize(SplitStoreType))
     return false;
 
   // Don't split the store if it is volatile.
   if (SI.isVolatile())
     return false;
 

llvm/test/CodeGen/X86/codegen_prepare_large_structs.ll

+; RUN: opt -codegenprepare -mtriple=x86_64-unknown-unknown -disable-output < %s
+
+%struct.large = type { [2500000 x i8] }
+
+define void @foo(%struct.large %s, %struct.large* %p) {
+  store %struct.large %s, %struct.large* %p, align 1
+  ret void
+}