[InstCombine] Add a cl::opt to control calls to getOrEnforceKnownAlignment in LoadInst and StoreInst

[0xdc03]

Summary

This is in preparation for the InferAlignment pass which handles
inferring alignment for instructions separately. It is better to handle
this as a separate pass as inferring alignment is quite costly, and
InstCombine running multiple times in the pass pipeline makes it even
more so.

[0xdc03]

llvm/test/Transforms/InstCombine/align-2d-gep.ll

11

I think this whole file can be removed, as its transferred to InferAlignment.

llvm/test/Transforms/InstCombine/constant-fold-gep.ll

117

I removed this test case because it will work under InferAlignment now.

[goldstein.w.n]

Imo this should be in a series with the InferAlignment pass rather than as a standalone patch.

Edit: typo

[nikic]

Thinking about how to stage these changes: I think ideally, we should add a cl::opt that controls whether a) the InferAlignment pass is enabled in PassBuilderPipelines and b) alignment inference in InstCombine is disabled. Then we can first land the new pass without enabling it, and then we can flip the switch to effectively switch from inference in InstCombine to the new pass. This will make it clear what the test impact (outside InstCombine) is. E.g. this patch currently fails clang tests, and it would be nice to see that those failures aren't present when the new pass is enabled at the same time.

[fhahn]

I think it would be good to try to asses the impact at least on the llvm-test-suite using statistics (see https://llvm.org/docs/TestSuiteGuide.html, TEST_SUITE_COLLECT_STATS) to see if there's any regressions for optimizations like DSE, GVN and others.

[0xdc03]

• Reorder patches, change to use a cl::opt

[0xdc03]

In D158527#4607641, @goldstein.w.n wrote:

Imo this should be in a series with the InferAlignment pass rather than as a standalone patch.

Edit: typo

Hmm, I had already put this in a stack before, is a series different from that?

In D158527#4609450, @fhahn wrote:

I think it would be good to try to asses the impact at least on the llvm-test-suite using statistics (see https://llvm.org/docs/TestSuiteGuide.html, TEST_SUITE_COLLECT_STATS) to see if there's any regressions for optimizations like DSE, GVN and others.

Will look into this.

[0xdc03]

Okay, @fhahn I have collected the results for both cases, can you please tell me which metrics to check?

[nikic]

The test updates here probably shouldn't be part of this patch, but rather part of the one flipping the flag?

[0xdc03]

In D158527#4614044, @nikic wrote:

The test updates here probably shouldn't be part of this patch, but rather part of the one flipping the flag?

Sure, I just thought they'd be easier to review this way, because that one consists entirely of automated changes.

[nikic]

In D158527#4610872, @0xdc03 wrote:

Okay, @fhahn I have collected the results for both cases, can you please tell me which metrics to check?

I'd just diff all the stats to start with. I use this script that: https://gist.github.com/nikic/812f9ac1a51e29ef453fed04a6d8c40f I'm not sure this will be particularly insightful for this patch.

Another approach is to look at IR diffs. I use this patch for that purpose: https://gist.github.com/nikic/da2c7ee8120c3e477f5afc662d531b66 And then commit the results for the baseline and the change and then git diff between them.

[0xdc03]

• Rebase on main

[nikic]

llvm/test/Transforms/LoopVectorize/non-const-n.ll

67 ↗

(On Diff #554223)

Can you please commit this test regeneration separately? (This was part of a UTC version 3 that was later reverted.)

[0xdc03]

In D158527#4616967, @nikic wrote:

In D158527#4610872, @0xdc03 wrote:

Okay, @fhahn I have collected the results for both cases, can you please tell me which metrics to check?

I'd just diff all the stats to start with. I use this script that: https://gist.github.com/nikic/812f9ac1a51e29ef453fed04a6d8c40f I'm not sure this will be particularly insightful for this patch.

Another approach is to look at IR diffs. I use this patch for that purpose: https://gist.github.com/nikic/da2c7ee8120c3e477f5afc662d531b66 And then commit the results for the baseline and the change and then git diff between them.

Okay, so I have these results for DSE and GVN (where the first column is *with* InferAlignment and the second is *without*):

dse.NumDomMemDefChecks                             | 364753 | 364783
dse.NumFastStores                                  | 7891   | 7890
dse.NumRedundantStores                             | 316    | 317
dse.NumRemainingStores                             | 369727 | 369726
gvn.IsValueFullyAvailableInBlockNumSpeculationsMax | 17650  | 17657
gvn.NumGVNInstr                                    | 122764 | 122765
gvn.NumPRELoad                                     | 14238  | 14239
gvn.NumPRELoadMoved2CEPred                         | 1451   | 1462

These are the individual differences (first value is *with* InferAlignment and second value is *without*):

MultiSource/Benchmarks/7zip/7zip-benchmark.test:
  dse.NumRemainingStores:                             32137.0 | 32135.0
  gvn.IsValueFullyAvailableInBlockNumSpeculationsMax: 1732.0  | 1729.0

MultiSource/Benchmarks/Ptrdist/yacr2/yacr2.test:
  gvn.NumPRELoadMoved2CEPred: 8.0 | 9.0

MultiSource/Applications/sqlite3/sqlite3.test:
  dse.NumFastStores:          152.0 | 151.0
  dse.NumRedundantStores:     29.0  | 30.0
  gvn.NumPRELoadMoved2CEPred: 150.0 | 152.0

MultiSource/Benchmarks/ASCI_Purple/SMG2000/smg2000.test:
  gvn.NumPRELoadMoved2CEPred: 4.0 | 5.0

MultiSource/Applications/aha/aha.test:
  dse.NumDomMemDefChecks:                             129.0 | 138.0
  dse.NumRemainingStores:                             89.0  | 90.0
  gvn.IsValueFullyAvailableInBlockNumSpeculationsMax: 19.0  | 20.0

MultiSource/Benchmarks/Trimaran/enc-3des/enc-3des.test:
  gvn.NumGVNInstr: 4.0 | 5.0

SingleSource/Benchmarks/Misc/whetstone.test:
  dse.NumDomMemDefChecks:                             50.0 | 71.0
  gvn.IsValueFullyAvailableInBlockNumSpeculationsMax: 1.0  | 0.0

SingleSource/Benchmarks/McGill/exptree.test:
  gvn.IsValueFullyAvailableInBlockNumSpeculationsMax: 7.0 | 6.0

[0xdc03]

• Remove unrelated test checks caused by regeneration

[nikic]

Those stats basically look like "no impact" to me. There's a mix of minor wins and losses. @fhahn Any concerns?

This implementation change here looks good to me, but I still think the test changes should be moved over to the last one in the series (though they are fine as well).

[0xdc03]

• Address reviewer comments
  • Move test changes to D158600

[nikic]

LGTM

[0xdc03]

• Rebase on main

[dyung]

Hi @0xdc03, this change seems to be causing build failures on several build bots. Can you take a look and revert if you need time to investigate?

• https://lab.llvm.org/buildbot/#/builders/139/builds/50025
• https://lab.llvm.org/buildbot/#/builders/216/builds/27554
• https://lab.llvm.org/buildbot/#/builders/217/builds/28482
• https://lab.llvm.org/buildbot/#/builders/243/builds/12947
• https://lab.llvm.org/buildbot/#/builders/86/builds/65367

[mstorsjo]

This broke building of the bugpoint executable:

ld.lld: error: undefined symbol: EnableInferAlignmentPass
>>> referenced by InstCombineLoadStoreAlloca.cpp
>>>               InstCombineLoadStoreAlloca.cpp.o:(llvm::InstCombinerImpl::visitLoadInst(llvm::LoadInst&)) in archive lib/libLLVMInstCombine.a
>>> referenced by InstCombineLoadStoreAlloca.cpp
>>>               InstCombineLoadStoreAlloca.cpp.o:(llvm::InstCombinerImpl::visitStoreInst(llvm::StoreInst&)) in archive lib/libLLVMInstCombine.a
collect2: error: ld returned 1 exit status

[0xdc03]

@dyung and @mstorsjo , sorry about that. I had a fix ready, it just took some time to test. Should be fixed with https://github.com/llvm/llvm-project/commit/515a8263269278466b4fbbf22073bc6f84e6fd70.

[mstorsjo]

In D158527#4648655, @0xdc03 wrote:

@dyung and @mstorsjo , sorry about that. I had a fix ready, it just took some time to test. Should be fixed with https://github.com/llvm/llvm-project/commit/515a8263269278466b4fbbf22073bc6f84e6fd70.

Thanks, that does seem to fix it in the cases where I ran into it.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

 STATISTIC(NumDeadStore, "Number of dead stores eliminated");
 STATISTIC(NumGlobalCopies, "Number of allocas copied from constant global");
 
 static cl::opt<unsigned> MaxCopiedFromConstantUsers(
     "instcombine-max-copied-from-constant-users", cl::init(300),
     cl::desc("Maximum users to visit in copy from constant transform"),
     cl::Hidden);
 
+extern cl::opt<bool> EnableInferAlignmentPass;
+
 /// isOnlyCopiedFromConstantMemory - Recursively walk the uses of a (derived)
 /// pointer to an alloca.  Ignore any reads of the pointer, return false if we
 /// see any stores or other unknown uses.  If we see pointer arithmetic, keep
 /// track of whether it moves the pointer (with IsOffset) but otherwise traverse
 /// the uses.  If we see a memcpy/memmove that targets an unoffseted pointer to
 /// the alloca, and if the source pointer is a pointer to a constant memory
 /// location, we can optimize this.
 static bool
Instruction *InstCombinerImpl::visitLoadInst(LoadInst &LI) {
   Value *Op = LI.getOperand(0);
   if (Value *Res = simplifyLoadInst(&LI, Op, SQ.getWithInstruction(&LI)))
     return replaceInstUsesWith(LI, Res);
 
   // Try to canonicalize the loaded type.
   if (Instruction *Res = combineLoadToOperationType(*this, LI))
     return Res;
 
+  if (!EnableInferAlignmentPass) {
     // Attempt to improve the alignment.
     Align KnownAlign = getOrEnforceKnownAlignment(
         Op, DL.getPrefTypeAlign(LI.getType()), DL, &LI, &AC, &DT);
     if (KnownAlign > LI.getAlign())
       LI.setAlignment(KnownAlign);
+  }
 
   // Replace GEP indices if possible.
   if (Instruction *NewGEPI = replaceGEPIdxWithZero(*this, Op, LI))
     return replaceOperand(LI, 0, NewGEPI);
 
   if (Instruction *Res = unpackLoadToAggregate(*this, LI))
     return Res;
 

 Instruction *InstCombinerImpl::visitStoreInst(StoreInst &SI) {
   Value *Val = SI.getOperand(0);
   Value *Ptr = SI.getOperand(1);
 
   // Try to canonicalize the stored type.
   if (combineStoreToValueType(*this, SI))
     return eraseInstFromFunction(SI);
 
+  if (!EnableInferAlignmentPass) {
     // Attempt to improve the alignment.
     const Align KnownAlign = getOrEnforceKnownAlignment(
         Ptr, DL.getPrefTypeAlign(Val->getType()), DL, &SI, &AC, &DT);
     if (KnownAlign > SI.getAlign())
       SI.setAlignment(KnownAlign);
+  }
 
   // Try to canonicalize the stored type.
   if (unpackStoreToAggregate(*this, SI))
     return eraseInstFromFunction(SI);
 
   if (removeBitcastsFromLoadStoreOnMinMax(*this, SI))
     return eraseInstFromFunction(SI);
 

llvm/test/Transforms/InstCombine/align-2d-gep.ll

-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -passes=instcombine -S | FileCheck %s
-target datalayout = "E-p:64:64:64-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128"
-
-; A multi-dimensional array in a nested loop doing vector stores that
-; aren't yet aligned. Instcombine can understand the addressing in the
-; Nice case to prove 16 byte alignment. In the Awkward case, the inner
-; array dimension is not even, so the stores to it won't always be
-; aligned. Instcombine should prove alignment in exactly one of the two
-; stores.
-
-@Nice    = global [1001 x [20000 x double]] zeroinitializer, align 32
-@Awkward = global [1001 x [20001 x double]] zeroinitializer, align 32
-
-define void @foo() nounwind  {
-; CHECK-LABEL: @foo(
-; CHECK-NEXT:  entry:
-; CHECK-NEXT:    br label [[BB7_OUTER:%.*]]
-; CHECK:       bb7.outer:
-; CHECK-NEXT:    [[I:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[INDVAR_NEXT26:%.*]], [[BB11:%.*]] ]
-; CHECK-NEXT:    br label [[BB1:%.*]]
-; CHECK:       bb1:
-; CHECK-NEXT:    [[J:%.*]] = phi i64 [ 0, [[BB7_OUTER]] ], [ [[INDVAR_NEXT:%.*]], [[BB1]] ]
-; CHECK-NEXT:    [[T4:%.*]] = getelementptr [1001 x [20000 x double]], ptr @Nice, i64 0, i64 [[I]], i64 [[J]]
-; CHECK-NEXT:    store <2 x double> zeroinitializer, ptr [[T4]], align 16
-; CHECK-NEXT:    [[S4:%.*]] = getelementptr [1001 x [20001 x double]], ptr @Awkward, i64 0, i64 [[I]], i64 [[J]]
-; CHECK-NEXT:    store <2 x double> zeroinitializer, ptr [[S4]], align 8
-; CHECK-NEXT:    [[INDVAR_NEXT]] = add i64 [[J]], 2
-; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp eq i64 [[INDVAR_NEXT]], 556
-; CHECK-NEXT:    br i1 [[EXITCOND]], label [[BB11]], label [[BB1]]
-; CHECK:       bb11:
-; CHECK-NEXT:    [[INDVAR_NEXT26]] = add i64 [[I]], 1
-; CHECK-NEXT:    [[EXITCOND27:%.*]] = icmp eq i64 [[INDVAR_NEXT26]], 991
-; CHECK-NEXT:    br i1 [[EXITCOND27]], label [[RETURN_SPLIT:%.*]], label [[BB7_OUTER]]
-; CHECK:       return.split:
-; CHECK-NEXT:    ret void
-;
-entry:
-  br label %bb7.outer
-
-bb7.outer:
-  %i = phi i64 [ 0, %entry ], [ %indvar.next26, %bb11 ]
-  br label %bb1
-
-bb1:
-  %j = phi i64 [ 0, %bb7.outer ], [ %indvar.next, %bb1 ]
-
-  %t4 = getelementptr [1001 x [20000 x double]], ptr @Nice, i64 0, i64 %i, i64 %j
-  store <2 x double><double 0.0, double 0.0>, ptr %t4, align 8
-
-  %s4 = getelementptr [1001 x [20001 x double]], ptr @Awkward, i64 0, i64 %i, i64 %j
-  store <2 x double><double 0.0, double 0.0>, ptr %s4, align 8
-
-  %indvar.next = add i64 %j, 2
-  %exitcond = icmp eq i64 %indvar.next, 556
-  br i1 %exitcond, label %bb11, label %bb1
-
-bb11:
-  %indvar.next26 = add i64 %i, 1
-  %exitcond27 = icmp eq i64 %indvar.next26, 991
-  br i1 %exitcond27, label %return.split, label %bb7.outer
-
-return.split:
-  ret void
-}

llvm/test/Transforms/InstCombine/align-addr.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
 target datalayout = "E-p:64:64:64-p1:32:32:32-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128"
 
-; Instcombine should be able to prove vector alignment in the
-; presence of a few mild address computation tricks.
-
 define void @test0(ptr %b, i64 %n, i64 %u, i64 %y) nounwind  {
 ; CHECK-LABEL: @test0(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[C:%.*]] = ptrtoint ptr [[B:%.*]] to i64
 ; CHECK-NEXT:    [[D:%.*]] = and i64 [[C]], -16
 ; CHECK-NEXT:    [[E:%.*]] = inttoptr i64 [[D]] to ptr
 ; CHECK-NEXT:    [[V:%.*]] = shl i64 [[U:%.*]], 1
 ; CHECK-NEXT:    [[Z:%.*]] = and i64 [[Y:%.*]], -2

llvm/test/Transforms/InstCombine/assume.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=instcombine -S  -instcombine-infinite-loop-threshold=2  | FileCheck --check-prefixes=CHECK,DEFAULT %s
 ; RUN: opt < %s -passes=instcombine --enable-knowledge-retention -S  -instcombine-infinite-loop-threshold=2  | FileCheck --check-prefixes=CHECK,BUNDLES %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 declare void @llvm.assume(i1) #1
 
-; Check that the alignment has been upgraded and that the assume has not
-; been removed:
+; Check that the assume has not been removed:
 
 define i32 @foo1(ptr %a) #0 {
 ; DEFAULT-LABEL: @foo1(
 ; DEFAULT-NEXT:    [[T0:%.*]] = load i32, ptr [[A:%.*]], align 32
 ; DEFAULT-NEXT:    [[PTRINT:%.*]] = ptrtoint ptr [[A]] to i64
 ; DEFAULT-NEXT:    [[MASKEDPTR:%.*]] = and i64 [[PTRINT]], 31
 ; DEFAULT-NEXT:    [[MASKCOND:%.*]] = icmp eq i64 [[MASKEDPTR]], 0
 ; DEFAULT-NEXT:    tail call void @llvm.assume(i1 [[MASKCOND]])

 ;
   tail call void @llvm.assume(i1 true) ["ignore"(ptr undef)]
   %load = load i32, ptr %P
   ret i32 %load
 }
 
 define i1 @nonnull1(ptr %a) {
 ; CHECK-LABEL: @nonnull1(
-; CHECK-NEXT:    [[LOAD:%.*]] = load ptr, ptr [[A:%.*]], align 8, !nonnull [[META6:![0-9]+]], !noundef [[META6]]
+; CHECK-NEXT:    [[LOAD:%.*]] = load ptr, ptr [[A:%.*]], align 8, !nonnull !6, !noundef !6
 ; CHECK-NEXT:    tail call void @escape(ptr nonnull [[LOAD]])
 ; CHECK-NEXT:    ret i1 false
 ;
   %load = load ptr, ptr %a
   %cmp = icmp ne ptr %load, null
   tail call void @llvm.assume(i1 %cmp)
   tail call void @escape(ptr %load)
   %rval = icmp eq ptr %load, null

llvm/test/Transforms/InstCombine/assume_inevitable.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
 
 ; Check that assume is propagated backwards through all
 ; operations that are `isGuaranteedToTransferExecutionToSuccessor`
-; (it should reach the load and mark it as `align 32`).
 define i32 @assume_inevitable(ptr %a, ptr %b, ptr %c) {
 ; CHECK-LABEL: @assume_inevitable(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[M:%.*]] = alloca i64, align 8
 ; CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[A:%.*]], align 32
 ; CHECK-NEXT:    [[LOADRES:%.*]] = load i32, ptr [[B:%.*]], align 4
 ; CHECK-NEXT:    [[LOADRES2:%.*]] = call i32 @llvm.annotation.i32.p0(i32 [[LOADRES]], ptr nonnull @.str, ptr nonnull @.str1, i32 2)
 ; CHECK-NEXT:    store i32 [[LOADRES2]], ptr [[A]], align 32

llvm/test/Transforms/InstCombine/constant-fold-gep.ll

entry:
   %B2 = ptrtoint ptr addrspace(1) @X_as1 to i16
   %C = sub i16 0, %B2
   %D = getelementptr i8, ptr addrspace(1) %A, i16 %C
   %E = ptrtoint ptr addrspace(1) %D to i16
 
   ret i16 %E
 }
 
-; Check that we improve the alignment information.
-; The base pointer is 16-byte aligned and we access the field at
-; an offset of 8-byte.
-; Every element in the @CallerInfos array is 16-byte aligned so
-; any access from the following gep is 8-byte aligned.
-%struct.CallerInfo = type { ptr, i32 }
-@CallerInfos = global [128 x %struct.CallerInfo] zeroinitializer, align 16
-
-define i32 @test_gep_in_struct(i64 %idx) {
-; CHECK-LABEL: @test_gep_in_struct(
-; CHECK-NEXT:    [[NS7:%.*]] = getelementptr inbounds [128 x %struct.CallerInfo], ptr @CallerInfos, i64 0, i64 [[IDX:%.*]], i32 1
-; CHECK-NEXT:    [[RES:%.*]] = load i32, ptr [[NS7]], align 8
-; CHECK-NEXT:    ret i32 [[RES]]
-;
-  %NS7 = getelementptr inbounds [128 x %struct.CallerInfo], ptr @CallerInfos, i64 0, i64 %idx, i32 1
-  %res = load i32, ptr %NS7, align 1
-  ret i32 %res
-}
-
 @g = external global i8
 @g2 = external global i8
 
 declare i64 @get.i64()
 declare void @use.ptr(ptr)
 
 define ptr @gep_sub_self() {
 ; CHECK-LABEL: @gep_sub_self(