[PATCH] D108734: [InstCombine] Replace icmp invariant group operands with the invariant group's operands

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek requested changes to this revision.
Prazek added a comment.
This revision now requires changes to proceed.

Lets talk offline about the specific case to get it right.  The model is pretty brittle in those areas, so we have to be sure the transformations are legal



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:5705-5717
+static Instruction *foldICmpInvariantGroup(ICmpInst &I) {
+  std::array<Value *, 2> Ops = {I.getOperand(0), I.getOperand(1)};
+  bool Changed = false;
+  for (Value *&Op : Ops) {
+    auto *I = dyn_cast<Instruction>(Op);
+    if (!I || !I->isLaunderOrStripInvariantGroup())
+      continue;
----------------
If I understand this correctly, this essentially strips launders/strips from cmp operands.

I think this is incorrect. From the perspective of devirtualization we can have code like:


  %vtable_a = load i8* %a, !invariant.group !{}
  ;;;
  ; %a == %b
  %addr_a = call i8* @llvm.strip.invariant.group(i8* %a)
  %addr_b = call i8* @llvm.strip.invariant.group(i8* %b)
  %bool = icmp %addr_a, %addr_b
  br %bool, %if, %after
  if:
  ; This will not be able to change %b to %a
  %vtable_b = load i8* %b, !invariant.group !{}


As you see, the two loads operate on %a and %b (where %b is a result of launder). As comparisons can leak the idea that %a and %b are essentially the same addresses (thus enabling GVN to RAUW %b with %a), we need to strip it before cmp. 

In this case, we would replace the cmp with icmp %a %b, which would essentially tell GVN %a and %b can be used interchanganbly in this branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108734