PNaCl: Add ExpandVarArgs pass for expanding out variable-args function calls

PNaCl: Add ExpandVarArgs pass for expanding out variable-args function calls

Once this pass is enabled, it will simplify the language to reduce the
set of constructs that a PNaCl translator needs to handle as part of a
stable wire format for PNaCl.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3338
TEST=test/Transforms/NaCl/expand-varargs.ll

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=7216560

[eliben, 2013-03-21 16:10:11]

Great work. The comments are mostly very minor.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
File lib/Transforms/NaCl/ExpandVarArgs.cpp (right):

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:67: static void ExpandVarArgFunc(Function
*Func) {
Since you only call this from runOnModule, why not pass the Module* in, instead
of ->getParent().

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:69: Type *PtrType =
Type::getInt8Ty(Module->getContext())->getPointerTo();
Type::getInt8PtrTy

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:72: std::vector<Type*>
Params(FTy->param_begin(), FTy->param_end());
Did you consider SmallVector? It can be more efficient here because it 99% of
the cases there won't be more than, say, 8 parameters

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:80:
Func->getParent()->getFunctionList().insert(Func, NewFunc);
Again, would be nice to just have a Module* here instead of repeatedly calling
Func->getParent

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:93: Func->replaceAllUsesWith(
Would it also be useful to preserve the debug information for the function?

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:119: Module *Module =
Inst->getParent()->getParent()->getParent();
Here it's probably even better reason to pass Module in, or Context if you only
need that.

Note that it's also fine to save the Module the pass is operating on in a
private member if you don't want to pass it around.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:134: Value *Next =
GetElementPtrInst::Create(CurrentPtr, Indexes,
Is it worthwhile to create such a GEP if you're going to expand it later anyway?
[btw, note this imposes a pass running order]

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:158: std::vector<Value*> Args;
Here and elsewhere, use SmallVector. It was created in LLVM to avoid heap
allocation for small std::vectors

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:178: static bool ExpandVarArgCall(InstType
*Call, DataLayout *DL) {
Please add a comment explaining what this does

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:230: APInt(64,
DL->getTypeAllocSize(VarArgsTy)));
I'm not sure you need APInts  here and elsewhere. Won't a uint64_t do?

https://codereview.chromium.org/12481021/diff/5001/test/Transforms/NaCl/expan...
File test/Transforms/NaCl/expand-varargs.ll (right):

https://codereview.chromium.org/12481021/diff/5001/test/Transforms/NaCl/expan...
test/Transforms/NaCl/expand-varargs.ll:1: ; RUN: opt < %s -expand-varargs -S |
FileCheck %s
Add a test with more than one call/invoke to a vararg in the same function

[Mark Seaborn, 2013-03-21 18:07:55]

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
File lib/Transforms/NaCl/ExpandVarArgs.cpp (right):

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:67: static void ExpandVarArgFunc(Function
*Func) {
On 2013/03/21 16:10:11, eliben wrote:
> Since you only call this from runOnModule, why not pass the Module* in,
> instead of ->getParent().

Using getParent() seems to be idiomatic in LLVM.  e.g. grep for
"getParent.*getParent".  It simplifies the call site, it reduces the chance of
having out-of-sync arguments, and it reduces the chance of ending up with an
unused argument after refactoring.

But I did notice that getContext() works at all levels of the hierarchy, not
just Modules, so that lets me remove many getParent()s.

As an aside, it's odd that there's no getModule() or getFunction() in addition
to getParent().

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:69: Type *PtrType =
Type::getInt8Ty(Module->getContext())->getPointerTo();
On 2013/03/21 16:10:11, eliben wrote:
> Type::getInt8PtrTy

Done.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:72: std::vector<Type*>
Params(FTy->param_begin(), FTy->param_end());
On 2013/03/21 16:10:11, eliben wrote:
> Did you consider SmallVector? It can be more efficient here because it 99% of
> the cases there won't be more than, say, 8 parameters

Done.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:80:
Func->getParent()->getFunctionList().insert(Func, NewFunc);
On 2013/03/21 16:10:11, eliben wrote:
> Again, would be nice to just have a Module* here instead of repeatedly calling
> Func->getParent

I don't think that's idiomatic.  Note that I based this code on
DeleteDeadVarargs() in lib/Transforms/IPO/DeadArgumentElimination.cpp: 
http://llvm.org/docs/doxygen/html/DeadArgumentElimination_8cpp_source.html

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:93: Func->replaceAllUsesWith(
On 2013/03/21 16:10:11, eliben wrote:
> Would it also be useful to preserve the debug information for the function?

Yes; I'll have to investigate this.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:119: Module *Module =
Inst->getParent()->getParent()->getParent();
On 2013/03/21 16:10:11, eliben wrote:
> Here it's probably even better reason to pass Module in, or Context if you
only
> need that.

Cleaned up to use Inst->getContext().

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:134: Value *Next =
GetElementPtrInst::Create(CurrentPtr, Indexes,
On 2013/03/21 16:10:11, eliben wrote:
> Is it worthwhile to create such a GEP if you're going to expand it later
anyway?
> [btw, note this imposes a pass running order]

It's certainly easier to use GEP and have a later pass expand it out for us. 
Note that we use a struct for packing the arguments when calling a varargs
function.

I think it's OK for the pass ordering to matter.  e.g. Whether we expand out
struct arguments first or varargs first will affect the calling convention for
how structs are passed through varargs.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:158: std::vector<Value*> Args;
On 2013/03/21 16:10:11, eliben wrote:
> Here and elsewhere, use SmallVector. It was created in LLVM to avoid heap
> allocation for small std::vectors

Done.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:178: static bool ExpandVarArgCall(InstType
*Call, DataLayout *DL) {
On 2013/03/21 16:10:11, eliben wrote:
> Please add a comment explaining what this does

Done.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:230: APInt(64,
DL->getTypeAllocSize(VarArgsTy)));
On 2013/03/21 16:10:11, eliben wrote:
> I'm not sure you need APInts  here and elsewhere. Won't a uint64_t do? 

There are two ways of creating an i64 ConstantInt:

ConstantInt::get(*Context, APInt(64, x))
ConstantInt::get(Type::getInt64Ty(*Context), x))

I don't know of a simpler way.

See http://llvm.org/docs/doxygen/html/classllvm_1_1ConstantInt.html

I originally did the second, but I think the APInt version is more concise.  I
can change it back if you'd like?

https://codereview.chromium.org/12481021/diff/5001/test/Transforms/NaCl/expan...
File test/Transforms/NaCl/expand-varargs.ll (right):

https://codereview.chromium.org/12481021/diff/5001/test/Transforms/NaCl/expan...
test/Transforms/NaCl/expand-varargs.ll:1: ; RUN: opt < %s -expand-varargs -S |
FileCheck %s
On 2013/03/21 16:10:11, eliben wrote:
> Add a test with more than one call/invoke to a vararg in the same function

Done.

[eliben, 2013-03-21 18:36:40]

lgtm

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
File lib/Transforms/NaCl/ExpandVarArgs.cpp (right):

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:67: static void ExpandVarArgFunc(Function
*Func) {
On 2013/03/21 18:07:55, Mark Seaborn wrote:
> On 2013/03/21 16:10:11, eliben wrote:
> > Since you only call this from runOnModule, why not pass the Module* in,
> > instead of ->getParent().
> 
> Using getParent() seems to be idiomatic in LLVM.  e.g. grep for
> "getParent.*getParent".  It simplifies the call site, it reduces the chance of
> having out-of-sync arguments, and it reduces the chance of ending up with an
> unused argument after refactoring.
> 

Alas, LLVM is full of cargo-cult programming so grepping is not an indication of
good practices. However, the reasons you provide make sense to me so I'm not
insisting.

> But I did notice that getContext() works at all levels of the hierarchy, not
> just Modules, so that lets me remove many getParent()s.
> 
> As an aside, it's odd that there's no getModule() or getFunction() in addition
> to getParent().

There are also some cases (see lib/Analysis/AliasAnalysisCounter.cpp and also
Lint.cpp) where the Module is saved to a member variable in the beginning of
runOn* and then used. This is shorter and likely more efficient than calling
getParent() many times.

I'm deferring it to your judgement what to choose. The code is good either way.

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:230: APInt(64,
DL->getTypeAllocSize(VarArgsTy)));
On 2013/03/21 18:07:55, Mark Seaborn wrote:
> On 2013/03/21 16:10:11, eliben wrote:
> > I'm not sure you need APInts  here and elsewhere. Won't a uint64_t do? 
> 
> There are two ways of creating an i64 ConstantInt:
> 
> ConstantInt::get(*Context, APInt(64, x))
> ConstantInt::get(Type::getInt64Ty(*Context), x))
> 
> I don't know of a simpler way.
> 
> See http://llvm.org/docs/doxygen/html/classllvm_1_1ConstantInt.html
> 
> I originally did the second, but I think the APInt version is more concise.  I
> can change it back if you'd like?

I initially thought that precomputing all useful types can be more efficient,
but then I looked in the code of ConstantInt::get and it calls itself with a
uint wrapped in APInt anyway (The member variable holding the value in
ConstantInt is a APInt), so it doesn't matter I guess

https://codereview.chromium.org/12481021/diff/10001/include/llvm/InitializePa...
File include/llvm/InitializePasses.h (right):

https://codereview.chromium.org/12481021/diff/10001/include/llvm/InitializePa...
include/llvm/InitializePasses.h:273: void
initializeExpandConstantExprPass(PassRegistry&); // @LOCALMOD
Just a thought: as this list keeps growing, perhaps we should take it out to a
separate include file? This should make merging easier.

https://codereview.chromium.org/12481021/diff/10001/lib/Transforms/NaCl/Expan...
File lib/Transforms/NaCl/ExpandVarArgs.cpp (right):

https://codereview.chromium.org/12481021/diff/10001/lib/Transforms/NaCl/Expan...
lib/Transforms/NaCl/ExpandVarArgs.cpp:72: SmallVector<Type*,8>
Params(FTy->param_begin(), FTy->param_end());
Idiomatic LLVM style whitespace is:

SmallVector<Type *, 8>

[not that I particularly love it, but it's the standard...]

https://codereview.chromium.org/12481021/diff/10001/lib/Transforms/NaCl/Expan...
lib/Transforms/NaCl/ExpandVarArgs.cpp:129: SmallVector<Value*,1> Indexes;
ditto

[Mark Seaborn, 2013-03-21 19:50:54]

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
File lib/Transforms/NaCl/ExpandVarArgs.cpp (right):

https://codereview.chromium.org/12481021/diff/5001/lib/Transforms/NaCl/Expand...
lib/Transforms/NaCl/ExpandVarArgs.cpp:93: Func->replaceAllUsesWith(
On 2013/03/21 18:07:55, Mark Seaborn wrote:
> On 2013/03/21 16:10:11, eliben wrote:
> > Would it also be useful to preserve the debug information for the function?
> 
> Yes; I'll have to investigate this.

AFAICT, replaceAllUsesWith() preserves the debug metadata's reference to the
function OK, and I see debug info generated in the assembly output OK.

I've added setDebugLoc() calls to preserve debug line numbers for the
instructions we rewrite.  Please take a look to see if the CopyDebug() helper is
OK with you.

https://codereview.chromium.org/12481021/diff/10001/lib/Transforms/NaCl/Expan...
File lib/Transforms/NaCl/ExpandVarArgs.cpp (right):

https://codereview.chromium.org/12481021/diff/10001/lib/Transforms/NaCl/Expan...
lib/Transforms/NaCl/ExpandVarArgs.cpp:72: SmallVector<Type*,8>
Params(FTy->param_begin(), FTy->param_end());
On 2013/03/21 18:36:40, eliben wrote:
> Idiomatic LLVM style whitespace is:
> 
> SmallVector<Type *, 8>
> 
> [not that I particularly love it, but it's the standard...]

OK, fixed.

https://codereview.chromium.org/12481021/diff/10001/lib/Transforms/NaCl/Expan...
lib/Transforms/NaCl/ExpandVarArgs.cpp:129: SmallVector<Value*,1> Indexes;
On 2013/03/21 18:36:40, eliben wrote:
> ditto

Done.