[PATCH] D139582: [GVN] Improve PRE on load instructions

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D139582#3999511 <https://reviews.llvm.org/D139582#3999511>, @Carrot wrote:

> The compile time regression on ClamAV is in file libclamav_htmlnorm.c.  The increased compile time is completely in GVN pass, from 4.0s to 5.3s. There is a huge function cli_html_normalise in this file, it's more than 6600 lines in generated assembly file. The statistic result of NumPRELoad increased from 1 to 10 because of this patch. In function GVNPass::runImpl we have
>
>   unsigned Iteration = 0;
>   while (ShouldContinue) {
>     LLVM_DEBUG(dbgs() << "GVN iteration: " << Iteration << "\n");
>     (void) Iteration;
>     ShouldContinue = iterateOnFunction(F);
>     Changed |= ShouldContinue;
>     ++Iteration;
>   }
>
> It means we continuously do GVN on a function until there is no more such optimization applicable. This patch enables more optimizations, potentially it may also cause more iterations on a function. In this case, the loop is executed 4 times without this patch, but 5 times with this patch. These numbers closely correlate to the increased compile time.
>
> So the extra compile time is caused by more iterations on a huge function, and the more iterations is caused by more optimizations enabled by this patch.

Thanks for the analysis. I think this is fine then. I expect https://reviews.llvm.org/D140097 to mitigate this somewhat.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:925
+/// NewValue.
+static void ReplaceValuesPerBlockEntry(
+    SmallVectorImpl<AvailableValueInBlock> &ValuesPerBlock, BasicBlock *BB,
----------------
nit: `replaceValuesPerBlockEntry`


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1392
+
+  for (Instruction &Inst : *SuccBB) {
+    if (Inst.isIdenticalTo(Load)) {
----------------
Block scan needs a cutoff.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1397
+      // be safely moved to PredBB.
+      if (Dep.isNonLocal())
+        return static_cast<LoadInst *>(&Inst);
----------------
Do we need to be concerned about speculating the load? You do perform a speculation check, but I think it does not cover the speculation of this load.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1398
+      if (Dep.isNonLocal())
+        return static_cast<LoadInst *>(&Inst);
+
----------------
`cast<>`


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2702
     for (auto *I : InstrsToErase) {
-      assert(I->getParent() == BB && "Removing instruction from wrong block?");
       LLVM_DEBUG(dbgs() << "GVN removed: " << *I << '\n');
----------------
I am not sure this assertion is safe to remove. I think a problem with your current code is that it may try to remove a load that is part of the leader table for that block, if that block has been processed before the current one.


================
Comment at: llvm/test/Transforms/GVN/PRE/pre-load.ll:766
+; critical edges. The other successors of those predecessors have same loads.
+; We can move all loads into predecessors.
+
----------------
Please pre-commit the test.


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

https://reviews.llvm.org/D139582