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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Imo this should be in a series with the InferAlignment pass rather than as a standalone patch.
Edit: typo
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.
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.
Hmm, I had already put this in a stack before, is a series different from that?
Will look into this.
Okay, @fhahn I have collected the results for both cases, can you please tell me which metrics to check?
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.
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.
llvm/test/Transforms/LoopVectorize/non-const-n.ll | ||
---|---|---|
67 | Can you please commit this test regeneration separately? (This was part of a UTC version 3 that was later reverted.) |
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
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).
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?
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
@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.
I think this whole file can be removed, as its transferred to InferAlignment.