[PATCH] D79650: [AssumeBundles] add cannonicalisation to the assume builder

[Tyker via Phabricator via llvm-commits]

Tyker added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:85
+/// canonical one. If it succeeds it will return that new knowledge otherwise it
+/// will return an emtpy knowledge.
+RetainedKnowledge canonicalizedKnowledge(RetainedKnowledge RK, Module *M) {
----------------
jdoerfert wrote:
> It will return \p RK not an empty one, right?
yes the comment was outdated.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:117
+}
+
 /// This class contain all knowledge that have been gather while building an
----------------
jdoerfert wrote:
> The above functions can be `static` I guess. I'm a little worried that we add yet another "stripPointer" function here. I guess you cannot reuse the existing ones but maybe we should extend those instead.
> 
> Why do you strip this again:
> `RK.WasOn = V->stripInBoundsOffsets();`
> That seems wrong. `V` is the base we can use, stripping dynamic inbounds offsets will invalidate the `deref` value, right?
> The above functions can be static I guess.
it is in an inline namespace so it is already implicitly static.

> I'm a little worried that we add yet another "stripPointer" function here. I guess you cannot reuse the 
> existing ones but maybe we should extend those instead.
Yes i agree that there are already implementations of this (which i partially ripped of) but they are not general enough for my use case because i need to update the alignment value during the stripping based on the instructions being stripped.

We should probably refactor the various striping and GetPointerBaseWithConstantOffset such that we have one generic implementation and all similar utilities are specialization of that generic implementation.

>Why do you strip this again:
>RK.WasOn = V->stripInBoundsOffsets();
>That seems wrong. V is the base we can use, stripping dynamic inbounds offsets will invalidate the deref value, right?
yeah we can't assume this because dynamic offsets maybe negative.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:168
+         (RK.WasOn->hasOneUse() &&
+          RK.WasOn->use_begin()->getUser() == InsertBeforeInstruction)))
+      return false;
----------------
jdoerfert wrote:
> I'm not super happy about the last part of the check but it is not incorrect. Let's keep it for now.
> 
> Btw. We can also weed out (if we don't do it already) the trivial cases:
> `deref` and `nonnull` for `alloca`, `byval` arguments, or certain global values for example.
i added a few rules to remove more useless knowledge to cover byval and globals.

> I'm not super happy about the last part of the check but it is not incorrect
can you elaborate on why ? the idea is that salvageKnowledge is intended to be called on an instruction about to be removed. and usually when removing an instruction we keep going recursively. this changed makes sure we don't preserve knowledge in a way that is going to prevent from removing instructions recursively.
we will need to deal with droppable uses in RecursivelyDeleteTriviallyDeadInstructions, isInstructionTriviallyDead and others. but there are also a few custom implementations of those.





================
Comment at: llvm/test/Analysis/BasicAA/featuretest.ll:41
 ; USE_ASSUME-NEXT:    store i32 7, i32* [[POINTER2]], align 4
-; USE_ASSUME-NEXT:    call void @llvm.assume(i1 true) [ "dereferenceable"(i32* [[POINTER]], i64 4), "nonnull"(i32* [[POINTER]]) ]
 ; USE_ASSUME-NEXT:    ret i32 0
----------------
jdoerfert wrote:
> Can you explain why we drop `nonnull` but not `deref` here? I mean, it is good but I don't get how. (As mentioned above, we could also drop deref).
nonnull and align is dropped because the canonicalizedKnowledge was able to canonicalize it through the inbounds gep of dynamic index and see that the underlying pointer is an alloca so it isn't worth preserving.

however i don't think it is possible to canonicalize a dereferencable through the inbounds gep of dynamic index since the index may be negative.

but i will add a GetUnderlyingObject to the check of whether the pointer is an alloca this way dereferencable will also be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79650