[PATCH] D82703: [InstCombine] convert assumes to operand bundles

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D82703#2503196 <https://reviews.llvm.org/D82703#2503196>, @Tyker wrote:

> In D82703#2489196 <https://reviews.llvm.org/D82703#2489196>, @jdoerfert wrote:
>
>> @Tyker are you going to upstream this? If not, can someone take over?
>
> I can do the upstreaming but i don't know what the current state is for upstreaming this.

I think there are different things in this patch, some can be upstreamed some need minor adjustment, and some I don't understand yet. See my comments. It would be great if we can finish this line of assume optimizations though.



================
Comment at: llvm/include/llvm/Transforms/Utils/AssumeBundleBuilder.h:67
+/// return a canonical RetainedKnowledge from RK, Assume holding RK,
+/// Will return a none RetainedKnowledge if RK is useless.
+RetainedKnowledge simplifyRetainedKnowledge(CallBase *Assume,
----------------
Nit: The first sentence is hard to read.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:756
+      return;
+  }
+
----------------
Why can we allow ephemeral here without potentially throwing away an assume that was used to argue it itself is useless? Should we not do the `AllowEphemeral` stuff for now?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1474
+    /// arguement and remove the assume if it becomes useless.
+    /// always returns nullptr for use as a return values.
+    auto RemoveConditionFromAssume = [&](Instruction *Assume) -> Instruction * {
----------------
Nit: Typo, arguement


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1578
+
+    /// Canonicalize Knowledge help in operand bundles.
+    if (II->hasOperandBundles()) {
----------------
Nit: typo: help


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1585
+          continue; // Prevent reducing knowledge in an align with offset since
+                    // extracting a RetainedKnowledge form then looses
+        RetainedKnowledge CanonRK = llvm::simplifyRetainedKnowledge(
----------------
losses what?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1593
+            return nullptr;
+          }
+          assert(RK.AttrKind == CanonRK.AttrKind);
----------------
I'm not sure why we return nullptr here and not continue going.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:315
+  if (!Assume)
+    return canonicalizedKnowledge(RK, Assume->getModule()->getDataLayout());
+  AssumeBuilderState Builder(Assume->getModule(), Assume, AC, DT);
----------------
`!Assume` means `Assume->GetModule()` will crash, probably add a DL argument.


================
Comment at: llvm/test/Analysis/ValueTracking/assume.ll:75
 ;
-  call void @llvm.assume(i1 true) ["dereferenceable"(i32* %0, i32 4)]
+  call void @llvm.assume(i1 true) ["dereferenceable"(i32* %0, i32 4), "align"(i32* %0, i32 8)]
   br i1 %cond, label %A, label %B
----------------
How do we get an alignment of 8 here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82703