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

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 added a comment.

Yeah, this is case when sinking is not very profitable.

I wrote patch to disable sinking  for these cases  long time ago - https://reviews.llvm.org/D53098 - but I am not sure why I abandoned it - probably LLVM was poor to infer nonnull/deref that time. I improved it a bit this summer.

Some open questions:

This patch adds llvm.assume so effectively it increases use count of a value and some optimizations under with hasOneUse() will not trigger. I would prefer just to disable sinking in this case.

fhahn:

> I got the first results back, for AArch64 cortex-a57, running the test-suite, spec2000 and > spec2k6. There is one regression (+10% runtime, +16% code size) in SPEC2006, and a handful of small improvements across suites (range -3% to -1%)



> So after a first look, it doesn't look too bad. I'll take a look into the SPEC2k6 regression. > > But it would be good to get numbers on other platforms/benchmarks too.



> BTW, I just made TryToSinkInstruction return false in all cases, to disable sinking.

(PR39119)

- Should we disable sinking? Or just disable sinking of call instructions?



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3157
+  if (auto CS = CallSite(I)) {
+    auto *CalledFunc = CS.getCalledFunction();
+    for (unsigned Idx = 0; Idx != CS.getNumArgOperands(); Idx++)
----------------
You need to check also llvm::NullPointerIsDefined(), I think


================
Comment at: llvm/test/Transforms/InstCombine/Assume-Remplacing-Call.ll:5
+
+define i32 @test_sink_remplacement1(i8* %p) {
+;CHECK-LABEL: @test_sink_remplacement1(
----------------
typo //remplacement//


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69477