[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 ↗

(On Diff #552423)

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

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

117 ↗

(On Diff #552423)

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);