[PATCH] D69477: [InstCombine] keep assumption before sinking calls

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

**TL;DR ** I would prefer a different solution but this seems fine for the short term. We could also get a similar short term solution up and running in the Attributor easily while avoiding some of the drawbacks I mention below, though that will have other consequences wrt. timing.

---

I personally would argue this is the wrong place to do this (in the middle-long term), assuming I grasp the whole scope of the change.

I'll outline what I think we should do (in the middle-long term) below. I also add incline comments for this approach if we want to stick with it for now (which I'm not actually opposing).

The problem here is more generic than `nonnull` in the sink transformation of instcombine.
The problem is that we would like to retain knowledge (of various kinds) while allowing transformations to do what they think is best.
Using `llvm.assume` is appealing for `nonnull` but why are we focusing on that one, e.g, why don't we save the `dereferenceability` information, `align`ment, ...?
Using `llvm.assume` is actually problematic for more complex cases, e.g., `dereferenceable` is hard to express.
So what I would like to see is a more generic framework to pin information/attributes to locations *only* if we, during a dedicated phase, decide that it might be profitable. 
Above I already tried to make the point why we don't want to use `llvm.assume`, now we should not do it on a "per-transformation-basis" because it is not clear if this is the right thing to do (I mean retain the information explicitly):

- What if in the tests above the argument `%p` of `test_sink_remplacementX` would be `nonnull`? (Right now we would introduce a useless `assume`.)
- What if there are (very likely) no users of the information tracked with the `llvm.assume`?
- What other passes need to be augmented or do we want to adopt a "case-by-case" approach?

To make the above scheme work we need to come up with a new "assumption encoding".
That said, there are various reasons to do so, e.g., `__builtin_assume` with potential side effects come to mind, so does `#pragma omp assume` (OpenMP TR8, to be released soon). 
I have some ideas on how this could look like without disturbing transformations or accidentally exposing side-effects, but so far I haven't put my thoughts into a coherent write-up.
If there is interest, please contact me :)



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3155
+  // If this instruction was providing nonnull guarantees insert assumptions for
+  // nonnull arguments.
+  if (auto CS = CallSite(I)) {
----------------
[Wording]

> // nonnull call site arguments.

While you only test caller arguments below, the code will actually add assumptions for any `nonnull` call site argument.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3163
+           (CS.paramHasAttr(Idx, Attribute::Dereferenceable) &&
+            CS.getAttributes().getParamDereferenceableBytes(Idx) > 0))) {
+        Value *V = CS.getArgument(Idx);
----------------
The above checks are "too complicated". You don't want to look at call site and callee explictly but use helpers that look at both. In fact, `CallBase::paramHasAttr` is such a helper that looks at the call base and the callee if available. Also, there is no `dereferenceable(0)` which should simplify the last part.

Btw. the comment by @xbolva00 is correct, deref doesn't always imply `nonnull`.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3169
+      }
+  }
+
----------------
Nit: Use a `CallBase` instead of `CallSite`


================
Comment at: llvm/test/Transforms/InstCombine/Assume-Remplacing-Call.ll:67
+;CHECK:   %call = call i64 @func_test1(i8* [[P]]) #2
+;CHECK: }
+
----------------
I would prefer using using the update test checks script.


================
Comment at: llvm/test/Transforms/InstCombine/Assume-Remplacing-Call.ll:88
+
+declare i64 @func_test1(i8* nonnull %0) #3
+
----------------
these names do not help, make the nonnull explicit in the name please.


================
Comment at: llvm/test/Transforms/InstCombine/Assume-Remplacing-Call.ll:89
+declare i64 @func_test1(i8* nonnull %0) #3
+
+attributes #1 = { readonly }
----------------
A test is missing where the sunk function is not in the entry block and where the argument of the sunk function is not an argument of the caller because in the cases above we could just annotate the argument of the caller as nonnull. (no need for the assume).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69477