[PATCH] D144445: [ConstantFold][InstSimplify] folding load for constant global patterened arrays and structs

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:781
+  // elements
+  if ((!CSTy || !CSTy->isPacked()) && !CATy)
+    return nullptr;
----------------
This looks like a leftover from a previous implementation? I don't think your current one has any limitations regarding the type of the initializer.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:786
+  // Bail for large initializers in excess of 64K to avoid allocating
+  // too much memory.
+  if (!GVSize || UINT16_MAX < GVSize)
----------------
If we have a 64K global and are reading 4 byte elements (i32) from it, then we need to check that 16k elements are the same. This is too expensive, as the compile-time regression shows.

We should limit this more aggressively to achieve reasonable compile-times, e.g. to 1K globals.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:794
+  unsigned char *GVBytes = RawBytes.data();
+  if (!ReadDataFromGlobal(C, 0, GVBytes, GVSize, DL))
+    return nullptr;
----------------
Rather than `ReadDataFromGlobal()`, we should use `ConstantFoldLoadFromConst()` here (or rather in the loop below). This has two advantages: It's going to handle all types (including pointer loads), and it allows an early bailout if not all elements are the same. Your current implementation will always read the entire global, even if it's clear after two elements that it's not uniform.

It will also take care of endianness itself.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:798
+  unsigned LoadSize = LoadTy->getScalarSizeInBits() / 8;
+  if (LoadAlign <= GVSize)
+    for (unsigned ByteOffset = LoadAlign, E = GVSize - LoadSize;
----------------
If the load alignment is used, you also need to make sure that the global alignment is `>=` the load alignment. Otherwise it doesn't really tell you anything.

Though I'm not sure we really want to use alignment to determine access stride -- this is pretty unusual. Based this on the getelementptr stride would be more typical and handle cases that alignment can't (e.g. non-power-of-two stride).


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:6640
+  if (!GV || !GV->isConstant() ||
+      (!GV->hasDefinitiveInitializer() && !GV->hasUniqueInitializer()))
     return nullptr;
----------------
hasUniqueInitializer() is only relevant if you want to modify an initializer, it should not be checked here.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:6651
+          GV->getInitializer(), LI->getType(), LI->getAlign().value(), Q.DL))
+    return C;
 
----------------
It might make sense to move this to the `else` of the below branch. If we know a constant offset, then we don't need to go through this code.


================
Comment at: llvm/test/Transforms/InstSimplify/load-patterned-aggregates.ll:3
 ; RUN: opt < %s -passes=instsimplify -S | FileCheck %s
 target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:32:32-n8:16:32"
 @constzeroarray = internal constant [4 x i32] zeroinitializer
----------------
Rather than hardcoding the data layout, it's better to do something like this:
```
; RUN: opt < %s -passes=instsimplify -S -data-layout="e" | FileCheck %s --check-prefixes=CHECK,LE
; RUN: opt < %s -passes=instsimplify -S -data-layout="E" | FileCheck %s --check-prefixes=CHECK,BE
```
This will check both little and big endian.


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

https://reviews.llvm.org/D144445