On Fri, Dec 9, 2016 at 10:30 AM, Friedman, Eli <efriedma at codeaurora.org> wrote:> Mmm... maybe. The part I really don't like is the implied store: there > are a lot of transformations which specifically reason about store > instructions, and they would all need to be fixed to specifically deal with > "alloca with an argument" so it doesn't block optimizations. Every feature > we add to the IR makes it more difficult to write IR transformations, and > this really doesn't seem to carry its own weight. >I don't feel like it complicates our model that much, though. Passes already have to know that uninitialized allocas contain undef, and with this change they can contain a value. That doesn't seem very surprising. It is a fair point that we'd have to add new cases to code like llvm::FindAvailableLoadedValue, which looks for StoreInst but not AllocaInst.> A couple of other thoughts I had: > 1. We could use metadata, e.g. "%px = alloca i32, align 4 !argument !i32 > %x", followed by a store. There's still a little pattern-matching involved > because the metadata is only a hint, but the intent in the IR is more > explicit. >This works, but it doesn't feel like it keeps simple things simple. I'll definitely implement this if people don't like my proposal.> 2. We could change the way clang generates function definitions: instead > of "define void @f(i32 %x)", generate "define void @f(i32* byval %px)" if > the value is going to end up on the stack. This should work without any > changes to the IR. This is a bad idea if we're trying to run optimizations > because it obscures data flow, but it seems reasonable at -O0. >This is problematic because it would break out "bitcode ABI" between optimized and debug builds. We've had problems in the past when the caller and callee disagree on whether an argument is a byval pointer or not, and instcombine will come along and try to simplify the casts: https://llvm.org/bugs/show_bug.cgi?id=21573 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161209/2e28d7b8/attachment.html>
On 12/9/2016 11:49 AM, Reid Kleckner wrote:> On Fri, Dec 9, 2016 at 10:30 AM, Friedman, Eli > <efriedma at codeaurora.org <mailto:efriedma at codeaurora.org>> wrote: > > Mmm... maybe. The part I really don't like is the implied store: > there are a lot of transformations which specifically reason about > store instructions, and they would all need to be fixed to > specifically deal with "alloca with an argument" so it doesn't > block optimizations. Every feature we add to the IR makes it more > difficult to write IR transformations, and this really doesn't > seem to carry its own weight. > > > I don't feel like it complicates our model that much, though. Passes > already have to know that uninitialized allocas contain undef, and > with this change they can contain a value. That doesn't seem very > surprising. It is a fair point that we'd have to add new cases to code > like llvm::FindAvailableLoadedValue, which looks for StoreInst but not > AllocaInst.It's more that that... I mean, obviously you have to fix all the places which assume allocas start off containing undef, but you're also introducing the potential for missed optimizations by making the store implicit. For example, consider: void g(int *x); __attribute((noinline)) void f(int y) { y = 10; g(&y); } void h() { f(134314); } The argument to f is dead; there are two stores to y, and instcombine will eliminate the first one. With extended allocas, we need a new form of dead store elimination which transforms "alloca i32, argument %x" -> "alloca i32".> 2. We could change the way clang generates function definitions: > instead of "define void @f(i32 %x)", generate "define void @f(i32* > byval %px)" if the value is going to end up on the stack. This > should work without any changes to the IR. This is a bad idea if > we're trying to run optimizations because it obscures data flow, > but it seems reasonable at -O0. > > > This is problematic because it would break out "bitcode ABI" between > optimized and debug builds. We've had problems in the past when the > caller and callee disagree on whether an argument is a byval pointer > or not, and instcombine will come along and try to simplify the casts: > https://llvm.org/bugs/show_bug.cgi?id=21573If instcombine is breaking this somehow, it's a bug; we're not supposed to eliminate bitcasts unless we can prove the signatures are equivalent. Granted, this is a complicated transform which has had a lot of bugs in the past. -Eli -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161209/6923999d/attachment.html>
----- Original Message -----> From: "Eli via llvm-dev Friedman" <llvm-dev at lists.llvm.org> > To: "Reid Kleckner" <rnk at google.com> > Cc: "llvm-dev" <llvm-dev at lists.llvm.org> > Sent: Friday, December 9, 2016 2:21:57 PM > Subject: Re: [llvm-dev] RFC: Adding argument allocas> On 12/9/2016 11:49 AM, Reid Kleckner wrote:> > On Fri, Dec 9, 2016 at 10:30 AM, Friedman, Eli < > > efriedma at codeaurora.org > wrote: >> > > Mmm... maybe. The part I really don't like is the implied store: > > > there are a lot of transformations which specifically reason > > > about > > > store instructions, and they would all need to be fixed to > > > specifically deal with "alloca with an argument" so it doesn't > > > block > > > optimizations. Every feature we add to the IR makes it more > > > difficult to write IR transformations, and this really doesn't > > > seem > > > to carry its own weight. > > > > > I don't feel like it complicates our model that much, though. > > Passes > > already have to know that uninitialized allocas contain undef, and > > with this change they can contain a value. That doesn't seem very > > surprising. It is a fair point that we'd have to add new cases to > > code like llvm::FindAvailableLoadedValue, which looks for StoreInst > > but not AllocaInst. > > It's more that that... I mean, obviously you have to fix all the > places which assume allocas start off containing undef, but you're > also introducing the potential for missed optimizations by making > the store implicit. For example, consider:> void g(int *x); > __attribute((noinline)) void f(int y) { > y = 10; > g(&y); > } > void h() { f(134314); }> The argument to f is dead; there are two stores to y, and instcombine > will eliminate the first one. With extended allocas, we need a new > form of dead store elimination which transforms "alloca i32, > argument %x" -> "alloca i32".This is a good point, but it does not seem complicated to implement. -Hal> > > 2. We could change the way clang generates function definitions: > > > instead of "define void @f(i32 %x)", generate "define void > > > @f(i32* > > > byval %px)" if the value is going to end up on the stack. This > > > should work without any changes to the IR. This is a bad idea if > > > we're trying to run optimizations because it obscures data flow, > > > but > > > it seems reasonable at -O0. > > > > > This is problematic because it would break out "bitcode ABI" > > between > > optimized and debug builds. We've had problems in the past when the > > caller and callee disagree on whether an argument is a byval > > pointer > > or not, and instcombine will come along and try to simplify the > > casts: https://llvm.org/bugs/show_bug.cgi?id=21573 > > If instcombine is breaking this somehow, it's a bug; we're not > supposed to eliminate bitcasts unless we can prove the signatures > are equivalent. Granted, this is a complicated transform which has > had a lot of bugs in the past.> -Eli > -- > Employee of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a > Linux Foundation Collaborative Project > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev-- Hal Finkel Lead, Compiler Technology and Programming Languages Leadership Computing Facility Argonne National Laboratory -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161210/4a69a971/attachment.html>