[PATCH] D87445: [InstCombine] Fix incorrect SimplifyWithOpReplaced transform (PR47322)

[Nikita Popov via Phabricator via llvm-commits]

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

This is a followup to D86834 <https://reviews.llvm.org/D86834>, which partially fixed this issue in InstSimplify. However, InstCombine repeats the same transform while dropping poison flags -- which does not cover cases where poison is introduced in some other way.

The fix here is a bit more comprehensive, because things are quite entangled, and it's hard to only partially address it without regressing optimization. There are really two changes here:

- Export the SimplifyWithOpReplaced API from InstSimplify, with an added AllowRefinement flag. For replacements inside the TrueVal we don't actually care whether refinement occurs or not, the replacement is always legal. This part of the transform is now done in InstSimplify only. (It should be noted that the current AllowRefinement check is not sufficient -- that's an issue we need to address separately.)
- Change the InstCombine fold to work by temporarily dropping poison generating flags, running the fold and then restoring the flags if it didn't work out. This will ensure that the InstCombine fold is correct as long as the InstSimplify fold is correct.

I hope this approach makes sense.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87445

Files:
  llvm/include/llvm/Analysis/InstructionSimplify.h
  llvm/lib/Analysis/InstructionSimplify.cpp
  llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
  llvm/test/Transforms/InstCombine/select.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D87445.290922.patch
Type: text/x-patch
Size: 9715 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200910/e3bbf14d/attachment.bin>