search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

CodeExtractor gives up on alloca used exclusively within extracted region #38414

Open Quuxplusone opened 6 years ago

Quuxplusone commented 6 years ago
Bugzilla Link PR39441
Status NEW
Importance P enhancement
Reported by Vedant Kumar (vsk@apple.com)
Reported on 2018-10-25 15:47:14 -0700
Last modified on 2018-10-26 14:09:54 -0700
Version trunk
Hardware PC All
CC efriedma@quicinc.com, hiraditya@msn.com, llvm-bugs@lists.llvm.org, rnk@google.com, sebpop@gmail.com, tejohnson@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

On Darwin, there is an extremely pervasive kind of cold code (error logging) which CodeExtractor has trouble with:

if.then3:                                         ; preds = %if.then
  %3 = tail call i8* @llvm.stacksave(), !dbg !5744
  %vla13 = alloca [2 x i8], align 16, !dbg !5744
  %vla13.sub = getelementptr inbounds [2 x i8], [2 x i8]* %vla13, i64 0, i64 0
  store i8 0, i8* %vla13.sub, align 16, !dbg !5750
  %numArgs.i = getelementptr inbounds [2 x i8], [2 x i8]* %vla13, i64 0, i64 1, !dbg !5750
  store i8 0, i8* %numArgs.i, align 1, !dbg !5750
  notail call void @_os_log_debug_impl(... %numArgs.i ...)
  call void @llvm.stackrestore(i8* %3), !dbg !5752
  br label %if.end, !dbg !5752

By default, extracting regions containing allocas is not enabled. In general this seems difficult to do. But here, all uses of the alloca are within the outlined region...

Can we do something simple to permit outlining in this scenario? Maybe check that the transitive closure of the alloca's uses all live within the alloca's block?

Quuxplusone commented 6 years ago

The property you want to check is that the alloca is between a stacksave/stackrestore pair. That said, once you have code to detect that, you might as well just use it to hoist the alloca to the entry block instead.

Quuxplusone commented 6 years ago 
(In reply to Eli Friedman from comment #1)
> The property you want to check is that the alloca is between a
> stacksave/stackrestore pair.  That said, once you have code to detect that,
> you might as well just use it to hoist the alloca to the entry block instead.

Thanks Eli, that makes sense.

I suppose if we hoist the alloca to the entry block of the outlined function,
we can eliminate the stack{save,restore} pair. I'll split that out into a
different patch.
Quuxplusone commented 6 years ago

Seems very useful to implement. Let me know if you need any help. Thanks for bringing this example.

Quuxplusone commented 6 years ago

I’ll send out a patch for this today. I tested it yesterday and found that it lets CodeExtractor successfully outline a lot of expensive logging code in some frameworks.

Quuxplusone commented 6 years ago

Why is this alloca a VLA in the first place? LLVM doesn't really like dynamic allocas. If you can figure out how to have this not be a dynamic alloca, you might get better code all around.

If you do hoist the alloca, I'd recommend making sure you replace the stacksave and restore with lifetime markers to try to save stack space.

Quuxplusone commented 6 years ago 
(In reply to Reid Kleckner from comment #5)
> Why is this alloca a VLA in the first place?

I'm not sure. I suspect it's because this logging routine was designed to work
in environments where malloc isn't available.

> LLVM doesn't really like
> dynamic allocas. If you can figure out how to have this not be a dynamic
> alloca, you might get better code all around.
>
> If you do hoist the alloca, I'd recommend making sure you replace the
> stacksave and restore with lifetime markers to try to save stack space.

Thanks, that's a good idea.