[PATCH] D154307: [InstructionSimplify] Avoid simplifying ICmp without parent

Fri Jul 7 01:59:13 PDT 2023

nikic added a comment.

In D154307#4468201 <https://reviews.llvm.org/D154307#4468201>, @fhahn wrote:

> In D154307#4467234 <https://reviews.llvm.org/D154307#4467234>, @nikic wrote:
>
>> I believe this is related to the "phi of op" feature in NewGVN: https://github.com/llvm/llvm-project/blob/6954cb54252b50df95eea05c513e739b3ad2c8a0/llvm/lib/Transforms/Scalar/NewGVN.cpp#L2767 These cloned instructions won't have a parent.
>>
>> I'm not entirely sure what to do here. I don't think that this fix is sufficient: If we need support for instructions without parent for NewGVN, then we need to revert @arsenm's change entirely, not just this part. Alternatively we need a different way to handle this on the NewGVN side. It may be possible to insert the instructions, as long as we are sure to later remove them again. I'm not sure if this would cause problems with NewGVNs design though, which is not supposed to do IR modifications during the analysis phase.
>
> NewGVN makes use of instructions without parents in multiple places; I'm surprised that there isn't more fallout from this.

This probably went unnoticed because we only assert that the instruction is inserted in the primary simplifyInstruction() entry point, but not the helpers for individual instruction types.

> I am not sure if inserting the instructions in the existing IR would work well, but inserting them in a temporary block may work as workaround.

This seems like it might result in incorrect folds based on dominance relationships, especially is the temporary block is not connected to the CFG.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154307