[PATCH] D69477: [InstCombine] keep assumption before skinking calls
Dávid Bolvanský via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Oct 27 10:36:45 PDT 2019
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
More information about the llvm-commits
mailing list