[PATCH] D73411: [InstCombine] Process newly inserted instructions in the correct order

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: spatel, lebedev.ri, xbolva00, efriedma.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

InstCombine operates on the basic premise that the operands of the currently processed instruction have already been simplified. It achieves this by pushing instructions to the worklist in reverse program order, so that instructions are popped off in program order. The worklist management in the main combining loop also makes sure to uphold this invariant.

However, the same is not true for all the code that is performing manual worklist management. The largest problem (addressed in this patch) are instructions inserted by InstCombine's IRBuilder. These will be pushed onto the worklist in order of insertion (generally matching program order), which means that a) the users of the original instruction will be visited first, as they are pushed later in the main loop and b) the newly inserted instructions will be visited in reverse program order.

This causes a number of problems: First, folds operate on instructions that have not had their operands simplified, which may result in optimizations being missed (ran into this in https://reviews.llvm.org/D72048#1800424, which was the original motivation for this patch). Additionally, this increases the amount of folds InstCombine has to perform, both within one iteration, and by increasing the number of total iterations.

This patch addresses the issue by adding a `Worklist.AddDeferred()` method, which is used for instructions inserted by IRBuilder. These will only be added to the real worklist after the combine finished, and in reverse order, so they will end up processed in program order. I should note that the same should also be done to nearly all other uses of `Worklist.Add()`, but I'm starting with just this occurrence, which has by far the largest test fallout.

Most of the test changes are due to https://bugs.llvm.org/show_bug.cgi?id=44521 or other cases where we don't canonicalize something. These are neutral. There are also a couple of regressions that I will highlight in comments...


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73411

Files:
  llvm/include/llvm/Transforms/InstCombine/InstCombineWorklist.h
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/Transforms/InstCombine/canonicalize-clamp-like-pattern-between-negative-and-positive-thresholds.ll
  llvm/test/Transforms/InstCombine/canonicalize-clamp-like-pattern-between-zero-and-positive-threshold.ll
  llvm/test/Transforms/InstCombine/demorgan.ll
  llvm/test/Transforms/InstCombine/div.ll
  llvm/test/Transforms/InstCombine/getelementptr.ll
  llvm/test/Transforms/InstCombine/load.ll
  llvm/test/Transforms/InstCombine/logical-select.ll
  llvm/test/Transforms/InstCombine/max-of-nots.ll
  llvm/test/Transforms/InstCombine/or.ll
  llvm/test/Transforms/InstCombine/pr38915.ll
  llvm/test/Transforms/InstCombine/pr44245.ll
  llvm/test/Transforms/InstCombine/select-cmp-br.ll
  llvm/test/Transforms/InstCombine/select-pr39595.ll
  llvm/test/Transforms/InstCombine/sub-ashr-and-to-icmp-select.ll
  llvm/test/Transforms/InstCombine/sub-ashr-or-to-icmp-select.ll
  llvm/test/Transforms/InstCombine/sub-gep.ll
  llvm/test/Transforms/InstCombine/sub-minmax.ll
  llvm/test/Transforms/InstCombine/vec_sext.ll
  llvm/test/Transforms/InstCombine/xor.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73411.240368.patch
Type: text/x-patch
Size: 66871 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200125/45366b65/attachment-0001.bin>