This is an archive of the discontinued LLVM Phabricator instance.

[CodeExtractor] Allow extracting allocas within simple stack{save,restore} pairs
Needs ReviewPublic

Authored by vsk on Oct 26 2018, 3:00 PM.

Details

Summary

This enables outlining of a pervasive kind of cold code (error logging)
on Darwin. Fixes llvm.org/PR39441.

Diff Detail

Event Timeline

vsk created this revision.Oct 26 2018, 3:00 PM

The approach seems fine.

It seems weird that this is relevant; VLAs with a fixed size should generally be rare... unless the error logging code is intentionally using a VLA to trick the compiler into dynamically allocating the space?

llvm/lib/Transforms/Utils/CodeExtractor.cpp
215

There's no reason to expect a stacksave has exactly one use in general. (For example, it could have two uses if jump threading cloned a stackrestore, or it could be dead if the use was optimized out.)

223

You need to check that the parameter of the stackrestore is the corresponding stacksave.

vsk updated this revision to Diff 171368.Oct 26 2018, 4:12 PM
  • Fix the issues Eli pointed out.

The source is essentially:

uint8_t alignas(16) Buf[__builtin_os_log_format_buffer_size(fmt, ##__VA_ARGS__)];

In general I don't think this has to have a fixed size? Maybe some other optimization was able to simplify the alloca.

__builtin_os_log_format_buffer_size is a compiler builtin which always returns a constant integer.

vsk added a comment.Oct 26 2018, 5:18 PM

__builtin_os_log_format_buffer_size is a compiler builtin which always returns a constant integer.

How were you able to tell? I couldn't parse this out from the BUILTIN declaration, but, I also don't have my head wrapped around the custom type-checking code for it.

At any rate, in clang's EmitAutoVarAlloca, arrays with a size given by this builtin have !Ty->isConstantSizeType(), hence the stack{save,restore}.

I'm not sure whether I need to keep digging into this. If this patch is only generally useful to handle VLAs, I think it'd still be worthwhile.

How were you able to tell?

Looked at the expansion in CGBuiltin.cpp; it returns a ConstantInt.

Eli points out that D53514 will improve the codegen for the use case I'm most interested in, but in a way that makes this patch a bit less relevant. While it should still be useful to deal with actual VLAs, I'll need some other solution to allow outlining calls to os_log.

Is this patch still useful now that D53514 has landed?

Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2020, 10:47 AM