[PATCH] D158181: [SCEVExpander] Fix incorrect reuse of more poisonous instructions (PR63763)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h:447
+  /// DropPoisonGeneratingInsts is populated with instructions for which
+  /// poison-generating flags should be dropped if the value is reused.
+  Value *FindValueInExprValueMap(
----------------
nit: should be -> must be?


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1503
+
+    // If the instruction can't create poison, we can recurse to its operands.
+    if (canCreatePoison(cast<Operator>(I), /*ConsiderFlagsAndMetadata*/ false))
----------------
nit: this comment might be better placed after the check if, we handles the opposite case described.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1832
+  SmallVector<Instruction *> DropPoisonGeneratingInsts;
+  return FindValueInExprValueMap(S, At, DropPoisonGeneratingInsts);
 }
----------------
It's safe to ignore `DropPoisonGeneratingInsts` here, as the function is only used in isHighCostExpansionHelper, right? 

It might be good to lock down the API to return a bool for `getRelatedExistingExpansion` (`hasRelatedExistingExpansion`?) to make sure the return value isn't used elsewhere in the future without dropping the flags?


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

https://reviews.llvm.org/D158181