Neil Henning via llvm-dev
2020-Jan-22 17:09 UTC
[llvm-dev] Inlining + CSE + restrict pointers == funtimes
Ok I think we have some common ground - CSE should choose the aliased pointer over the non-aliased one because we don't want the no-aliasing information to creep outwards from the inlined callsite. I'll put together a patch in the coming days and add y'all as reviewers so you get visibility. Cheers, -Neil. On Wed, Jan 22, 2020 at 4:47 PM Jeroen Dobbelaere < Jeroen.Dobbelaere at synopsys.com> wrote:> That's indeed a good example that shows that dropping the metadata (A) is > the only correct solution. > > > > Greetings, > > > > Jeroen Dobbelaere > > > > > > *From:* Finkel, Hal J. <hfinkel at anl.gov> > *Sent:* Wednesday, January 22, 2020 17:33 > *To:* Jeroen Dobbelaere <dobbel at synopsys.com>; Neil Henning < > neil.henning at unity3d.com>; llvm-dev at lists.llvm.org > *Subject:* Re: [llvm-dev] Inlining + CSE + restrict pointers == funtimes > > > > At a high level, EarlyCSE should be intersecting the metadata of > instructions that it combines. If it doesn't, and also doesn't drop the > metadata, that seems like a bug, regardless of anything else. > > On 1/22/20 9:30 AM, Jeroen Dobbelaere wrote: > > [..] > > -- (A) dropping the noalias information will result in less optimization > opportunities > > -- (B) preferring the load with the noalias annotation can allow more > reorderings and a better schedule. > > (especially when there would be a 'store' in the context where the > restrict is valid; aka in 'called'). > > > > My preference would go to (B) as that opens up more optimization > opportunities. > > > > I'm concerned that this isn't sound. So, imagine if I had something like > this: > > int * restrict r = a; > ... > int x = noaliasing ? *r : *a; > > then we need the aliasing load to retain its dependencies relative to > other things in the block. If we combine them into one load, it needs to be > the aliasing one. > > Thanks again, > > Hal > > [...] > >-- Neil Henning Senior Software Engineer Compiler unity.com -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200122/b468fd5f/attachment.html>
Finkel, Hal J. via llvm-dev
2020-Jan-22 17:13 UTC
[llvm-dev] Inlining + CSE + restrict pointers == funtimes
Thanks, Neil. To be clear, the patch should cause the intersect function to be called on the metadata. -Hal On 1/22/20 11:09 AM, Neil Henning wrote: Ok I think we have some common ground - CSE should choose the aliased pointer over the non-aliased one because we don't want the no-aliasing information to creep outwards from the inlined callsite. I'll put together a patch in the coming days and add y'all as reviewers so you get visibility. Cheers, -Neil. On Wed, Jan 22, 2020 at 4:47 PM Jeroen Dobbelaere <Jeroen.Dobbelaere at synopsys.com<mailto:Jeroen.Dobbelaere at synopsys.com>> wrote: That's indeed a good example that shows that dropping the metadata (A) is the only correct solution. Greetings, Jeroen Dobbelaere From: Finkel, Hal J. <hfinkel at anl.gov<mailto:hfinkel at anl.gov>> Sent: Wednesday, January 22, 2020 17:33 To: Jeroen Dobbelaere <dobbel at synopsys.com<mailto:dobbel at synopsys.com>>; Neil Henning <neil.henning at unity3d.com<mailto:neil.henning at unity3d.com>>; llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org> Subject: Re: [llvm-dev] Inlining + CSE + restrict pointers == funtimes At a high level, EarlyCSE should be intersecting the metadata of instructions that it combines. If it doesn't, and also doesn't drop the metadata, that seems like a bug, regardless of anything else. On 1/22/20 9:30 AM, Jeroen Dobbelaere wrote: [..] -- (A) dropping the noalias information will result in less optimization opportunities -- (B) preferring the load with the noalias annotation can allow more reorderings and a better schedule. (especially when there would be a 'store' in the context where the restrict is valid; aka in 'called'). My preference would go to (B) as that opens up more optimization opportunities. I'm concerned that this isn't sound. So, imagine if I had something like this: int * restrict r = a; ... int x = noaliasing ? *r : *a; then we need the aliasing load to retain its dependencies relative to other things in the block. If we combine them into one load, it needs to be the aliasing one. Thanks again, Hal [...] -- [https://unity3d.com/profiles/unity3d/themes/unity/images/ui/other/unity-logo-dark-email.png] Neil Henning Senior Software Engineer Compiler unity.com<http://unity.com> -- 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/20200122/adf935dd/attachment-0001.html>
Neil Henning via llvm-dev
2020-Jan-24 11:52 UTC
[llvm-dev] Inlining + CSE + restrict pointers == funtimes
I've put the change up here https://reviews.llvm.org/D73342 for those interested. Cheers, -Neil. On Wed, Jan 22, 2020 at 5:13 PM Finkel, Hal J. <hfinkel at anl.gov> wrote:> Thanks, Neil. To be clear, the patch should cause the intersect function > to be called on the metadata. > > -Hal > On 1/22/20 11:09 AM, Neil Henning wrote: > > Ok I think we have some common ground - CSE should choose the aliased > pointer over the non-aliased one because we don't want the no-aliasing > information to creep outwards from the inlined callsite. > > I'll put together a patch in the coming days and add y'all as reviewers so > you get visibility. > > Cheers, > -Neil. > > On Wed, Jan 22, 2020 at 4:47 PM Jeroen Dobbelaere < > Jeroen.Dobbelaere at synopsys.com> wrote: > >> That's indeed a good example that shows that dropping the metadata (A) is >> the only correct solution. >> >> >> >> Greetings, >> >> >> >> Jeroen Dobbelaere >> >> >> >> >> >> *From:* Finkel, Hal J. <hfinkel at anl.gov> >> *Sent:* Wednesday, January 22, 2020 17:33 >> *To:* Jeroen Dobbelaere <dobbel at synopsys.com>; Neil Henning < >> neil.henning at unity3d.com>; llvm-dev at lists.llvm.org >> *Subject:* Re: [llvm-dev] Inlining + CSE + restrict pointers == funtimes >> >> >> >> At a high level, EarlyCSE should be intersecting the metadata of >> instructions that it combines. If it doesn't, and also doesn't drop the >> metadata, that seems like a bug, regardless of anything else. >> >> On 1/22/20 9:30 AM, Jeroen Dobbelaere wrote: >> >> [..] >> >> -- (A) dropping the noalias information will result in less optimization >> opportunities >> >> -- (B) preferring the load with the noalias annotation can allow more >> reorderings and a better schedule. >> >> (especially when there would be a 'store' in the context where the >> restrict is valid; aka in 'called'). >> >> >> >> My preference would go to (B) as that opens up more optimization >> opportunities. >> >> >> >> I'm concerned that this isn't sound. So, imagine if I had something like >> this: >> >> int * restrict r = a; >> ... >> int x = noaliasing ? *r : *a; >> >> then we need the aliasing load to retain its dependencies relative to >> other things in the block. If we combine them into one load, it needs to be >> the aliasing one. >> >> Thanks again, >> >> Hal >> >> [...] >> >> > > -- > Neil Henning > Senior Software Engineer Compiler > unity.com > > -- > Hal Finkel > Lead, Compiler Technology and Programming Languages > Leadership Computing Facility > Argonne National Laboratory > >-- Neil Henning Senior Software Engineer Compiler unity.com -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200124/f1a4a062/attachment.html>