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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

I left two comments. Could you verify the Value.h change does not impact compile time.  Other than than, LGTM.



================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:168
+         (RK.WasOn->hasOneUse() &&
+          RK.WasOn->use_begin()->getUser() == InsertBeforeInstruction)))
+      return false;
----------------
Tyker wrote:
> 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.
> 
> 
> 
> can you elaborate on why ?

Maybe I am wrong but I thought there is no enforced connection between `InsertBeforeInstruction` and the knowledge (or the instruction from which we retain knowledge). We use it a certain way, true, but my concerns are with potential other usages. As I said, it is not wrong and what I fear might never happen.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:136
+      if (isa<AllocaInst>(UnderlyingPtr) || isa<GlobalValue>(UnderlyingPtr))
+        return false;
+    }
----------------
If the kind is Deref you can ask the value for the known deref bytes with `Value::getPointerDereferenceableBytes`. For `align` it is `Value::getPointerAlignment`. Unsure if that is needed.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79650