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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Nice. I guess we should explicitly look into tramp3d afterwards, 20% probably offers nice opportunities to reduce the cost ;)

Some comments and questions below.



================
Comment at: llvm/lib/IR/Operator.cpp:46
+  return Result;
+}
+
----------------
The maximum alignment is `MaximumAlignment` in Value.h, please use that over the shifting stuff.

Can you explain the logic here in a comment at the function entry, please.


================
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) {
----------------
It will return \p RK not an empty one, right?


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:117
+}
+
 /// This class contain all knowledge that have been gather while building an
----------------
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?


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


================
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
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79650