nlewycky added a comment.
For the IR level, I think this has got to be valid:
declare void @bar(i32* byval %p)
define void @foo(i32* byval %p) {
tail call void @bar(i32* byval %p.tmp)
ret void
}
The `tail` is an aliasing property which indicates that the callee doesn't
touch any o
This is the IR I see in today’s trunk:
*** IR Dump Before Module Verifier ***
; Function Attrs: noinline optsize ssp
define i32 @_ZThn4_N1C4SeekE6_LARGE(%class.C* %this, %union._LARGE* byval align
4) unnamed_addr #2 align 2 {
entry:
%L = alloca %union._LARGE, align 8
%this.addr = alloca %clas
rnk added a comment.
So, if clang were to use a temporary alloca for the byval parameter, then yes,
I agree marking it as a tail call would be incorrect. However, clang doesn't
use an alloca, it forwards the byval pointer parameter directly to the callee:
define i32 @_ZThn4_N1C4SeekE6_LARGE(%
I forwarded your the thread for the DSE path. Even my original intent for was a
short-term bandage and by no means a long term fix.
> On Aug 1, 2016, at 10:53 AM, Reid Kleckner wrote:
>
> rnk added a comment.
>
> I think the correctness problem is in DSE. It should be safe for clang to put
>
rnk added a comment.
I think Nick was responsible for adding this DSE optimization *years* ago.
https://reviews.llvm.org/D22900
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
rnk added a comment.
I think the correctness problem is in DSE. It should be safe for clang to put
'tail' here. The tail call does not capture the address of any local variables,
but it does load from them via 'byval'.
https://reviews.llvm.org/D22900
On Sun, Jul 31, 2016 at 10:32 PM, Gerolf Hoflehner
wrote:
>
> > On Jul 31, 2016, at 1:46 AM, Amjad Aboud wrote:
> >
> > aaboud added a comment.
> >
> >> ISTM that the DWARF spec intended such thunks to be encoded as
> `DW_AT_trampoline`. That seems more appropriate than relying on codegen
> emi
> On Jul 31, 2016, at 12:11 AM, Amjad Aboud wrote:
>
> aaboud added a comment.
>
> Reverting https://reviews.llvm.org/rL244207, would be fine if we assure that
> PR24235, is fixed in another way.
I think it is inappropriate for anyone who introduced a bug to condition a
revert on a correct i
> On Jul 31, 2016, at 1:46 AM, Amjad Aboud wrote:
>
> aaboud added a comment.
>
>> ISTM that the DWARF spec intended such thunks to be encoded as
>> `DW_AT_trampoline`. That seems more appropriate than relying on codegen
>> emitting a tailcall. This way the debugger can make the policy deci
aaboud added a comment.
> ISTM that the DWARF spec intended such thunks to be encoded as
> `DW_AT_trampoline`. That seems more appropriate than relying on codegen
> emitting a tailcall. This way the debugger can make the policy decision of
> whether or not thunks should show up in the backtra
majnemer added a comment.
In https://reviews.llvm.org/D22900#501597, @aaboud wrote:
> Reverting https://reviews.llvm.org/rL244207, would be fine if we assure that
> PR24235, is fixed in another way.
> I added Reid who helped reviewing the original patch
> https://reviews.llvm.org/D11476.
IST
aaboud added a comment.
Reverting https://reviews.llvm.org/rL244207, would be fine if we assure that
PR24235, is fixed in another way.
I added Reid who helped reviewing the original patch
https://reviews.llvm.org/D11476.
https://reviews.llvm.org/D22900
__
I care highly about my commits. It's just that it's not actually my commit,
I committed it on Amjad's behalf before he had commit permissions. :-)
I think Eli missed the "Patch by" line when he originally added me to the
thread.
On Fri, Jul 29, 2016 at 11:16 AM, Gerolf Hoflehner
wrote:
> Sounds
Sounds like you care highly about your commit :-)
> On Jul 29, 2016, at 10:10 AM, Michael Kuperstein wrote:
>
> mkuper resigned from this revision.
> mkuper removed a reviewer: mkuper.
> mkuper added a comment.
>
> I really don't understand anything about this. :-)
>
>
> https://reviews.ll
mkuper resigned from this revision.
mkuper removed a reviewer: mkuper.
mkuper added a comment.
I really don't understand anything about this. :-)
https://reviews.llvm.org/D22900
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.ll
Gerolf updated this revision to Diff 66075.
Gerolf added a comment.
Reduced test case.
https://reviews.llvm.org/D22900
Files:
lib/CodeGen/CGVTables.cpp
test/CodeGenCXX/microsoft-abi-structors.cpp
test/CodeGenCXX/tail-byval.cpp
Index: test/CodeGenCXX/tail-byval.cpp
===
majnemer added a comment.
In https://reviews.llvm.org/D22900#498793, @Gerolf wrote:
> Nope, I don't see the tail call. Anyway, I'll simplify my test case. Don't
> worry about it.
>
> clang++ -cc1 -x c++ -emit-llvm -triple i386-apple-darwin9 t.cpp
>
> cat t.ll:
>
> ; ModuleID = 't.cpp'
> source_
Gerolf added a comment.
Nope, I don't see the tail call. Anyway, I'll simplify my test case. Don't
worry about it.
clang++ -cc1 -x c++ -emit-llvm -triple i386-apple-darwin9 t.cpp
cat t.ll:
; ModuleID = 't.cpp'
source_filename = "t.cpp"
target datalayout = "e-m:o-p:32:32-f64:32:64-f80:128-n8:16
majnemer added a comment.
In https://reviews.llvm.org/D22900#498781, @Gerolf wrote:
> Please add the options you used to compile? I can certainly shrink the test
> case a bit before I commit.
clang -cc1 -x c++ -emit-llvm -triple i386-apple-darwin9 t.ii -o -
https://reviews.llvm.org/D22900
Gerolf added a comment.
Please add the options you used to compile? I can certainly shrink the test
case a bit before I commit.
https://reviews.llvm.org/D22900
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/ma
majnemer added a subscriber: majnemer.
majnemer added a comment.
The test seems a little large, the following shows that we emit a tail call
with a byval argument on trunk.
struct LARGE {
union {
int i;
};
};
struct I {
virtual void m_fn1(LARGE);
};
struct CBase {
Gerolf created this revision.
Gerolf added reviewers: eli.friedman, mkuper.
Gerolf added a subscriber: cfe-commits.
This is just closing the loop for
https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg28837.html with a
test case and fixes PR28748 which had been introduced by r244207.
http
22 matches
Mail list logo
