[PATCH] D99027: [InstCombine] Whitelist non-refining folds in SimplifyWithOpReplaced

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:3935-3936
+  if (!AllowRefinement) {
+    // General InstSimplify functions may refine the result. To avoid this,
+    // implement only a few non-refining but profitable transforms here.
+
----------------
spatel wrote:
> I lost track of exactly what we mean by "refinement". The existing example in the code comment below + the test diffs in this patch suggest it is something like:
> "Substitution with another value can't let poison flow where it did not before." ?
> Are we getting in trouble with selects only or are there other opcodes with problems?
> 
> It can be independent of this patch, but we should make "refinement" clearer in the code comments here or in the header file comment where SimplifyWithOpReplaced() is declared.
Refinement means making a replacement like poison -> 0. This is legal. The select transform in question will take the simplification result (0) and replace it with the original instruction (poison). If the original simplification was a refinement, then this is a de-refinement, which is not legal. We can only make replacements that are valid in both directions, which requires the simplification result to be identical to the input. I believe the select optimization is the only place where we try to do something like this.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4042
 
       return ConstantFoldInstOperands(I, ConstOps, Q.DL, Q.TLI);
     }
----------------
aqjune wrote:
> This might make the result more refined as well, is it? ConstantFoldBinaryInstruction may fold undef/poison into a well-defined value.
> I think it is better to leave a comment here saying the issue.
There is a TODO For this in the canCreatePoison comment, but it ended up a bit far from the code it affects. I've moved it directly before the constant folding code now, which is hopefully clearer.


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

https://reviews.llvm.org/D99027