You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by Henry Robinson <he...@cloudera.com> on 2016/02/17 19:08:46 UTC
Re: Passing DataLayout to Module/Function Pass Manager of LLVM
(Moving this conversation to dev@impala.incubator.apache.org,
impala-dev@cloudera.org to bcc:)
On 17 February 2016 at 09:13, Silvius Rus <sr...@cloudera.com> wrote:
> Hey Nishidha,
>
> Would you be interested in contributing the changes you've made so far to
> the Impala ASF project? It's easier to get help in context if you post a
> patch for review. Upgrading to LLVM 3.7 would be beneficial to the project
> in general, so I think others will jump in to help.
>
> https://github.com/cloudera/Impala/wiki/Contributing-to-Impala
>
> Silvius
>
> On Wed, Feb 17, 2016 at 5:00 AM, nishidha panpaliya <ni...@gmail.com>
> wrote:
>
>> Alright Tim. Thanks for the reply.
>>
>> Regards,
>> Nishidha
>>
>>
>> On Tuesday, 16 February 2016 19:16:34 UTC+5:30, nishidha panpaliya wrote:
>>>
>>> Hello,
>>>
>>> In Impala source code, I've encountered a place in
>>> be/src/codegen/llvm-codegen.cc where DataLayout's pointer is passed to
>>> PassManager::addPass method.
>>>
>>> module_pass_manager->add(new DataLayout(data_layout_str));
>>> fn_pass_manager->add(new DataLayout(data_layout_str));
>>>
>>>
>>> *Note*: addPass method is also changed to add in LLVM 3.7.
>>>
>>> I wanted to understand what exactly these two lines are doing. What
>>> would be the impact if they are commented/removed? And if there is any test
>>> coverage for this?
>>>
>>> My rationale behind all these questions is to port Impala on ppc64le,
>>> I'd to upgrade LLVM 3.3 to 3.7. And in LLVM 3.7, DataLayout is not derived
>>> from class Pass and hence these lines do not compile with LLVM 3.7. So, I
>>> need to either find equivalent of this or have to remove.
>>>
>>> I'll be grateful to you if you could provide some insights here.
>>>
>>> Thanks in advance,
>>> Nishidha
>>>
>>> --
>> You received this message because you are subscribed to the Google Groups
>> "Impala Dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to impala-dev+unsubscribe@cloudera.org.
>>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Impala Dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to impala-dev+unsubscribe@cloudera.org.
>
--
Henry Robinson
Software Engineer
Cloudera
415-994-6679
Re: Passing DataLayout to Module/Function Pass Manager of LLVM
Posted by Nishidha Panpaliya <ni...@us.ibm.com>.
Hi Todd,
Thanks a lot. This really helped me.
So, in case I can simply ignore module->setDataLayout(...) as it is already
set in it once. In my case, with LLVM 3.3, datalayout was added as Pass to
ModulePassManager and FunctionPassManager, which now is not needed, as it
is already set in the module object being used by these passmanagers.
Please confirm my understanding.
Also, it seems in your module_builder.cc, you are continuing using
PassManagers which are made as legacy now in LLVM 3.7. I hope it is okay to
use those legacy PassManagers instead of new PassManagers in
IR/PassManager.h.
Thanks,
Nishidha
From: Todd Lipcon <to...@cloudera.com>
To: dev@impala.incubator.apache.org
Cc: Silvius Rus <sr...@cloudera.com>, nishidha panpaliya
<ni...@gmail.com>, Nishidha
Panpaliya/Austin/Contr/IBM@IBMUS, Sudarshan
Jagadale/Austin/Contr/IBM@IBMUS
Date: 02/17/2016 11:45 PM
Subject: Re: Passing DataLayout to Module/Function Pass Manager of LLVM
Regarding the DataLayout question, here's how we handle it with LLVM
3.7 in Kudu:
https://github.com/cloudera/kudu/blob/master/src/kudu/codegen/module_builder.cc#L267
You can see the diff in this mega commit:
https://github.com/cloudera/kudu/commit/9806863e78107505a622b44112a897189d9b3c24
On Wed, Feb 17, 2016 at 10:08 AM, Henry Robinson <he...@cloudera.com>
wrote:
> (Moving this conversation to dev@impala.incubator.apache.org,
> impala-dev@cloudera.org to bcc:)
>
> On 17 February 2016 at 09:13, Silvius Rus <sr...@cloudera.com> wrote:
>
>> Hey Nishidha,
>>
>> Would you be interested in contributing the changes you've made so far
to
>> the Impala ASF project? It's easier to get help in context if you post
a
>> patch for review. Upgrading to LLVM 3.7 would be beneficial to the
project
>> in general, so I think others will jump in to help.
>>
>> https://github.com/cloudera/Impala/wiki/Contributing-to-Impala
>>
>> Silvius
>>
>> On Wed, Feb 17, 2016 at 5:00 AM, nishidha panpaliya
<ni...@gmail.com>
>> wrote:
>>
>>> Alright Tim. Thanks for the reply.
>>>
>>> Regards,
>>> Nishidha
>>>
>>>
>>> On Tuesday, 16 February 2016 19:16:34 UTC+5:30, nishidha panpaliya
wrote:
>>>>
>>>> Hello,
>>>>
>>>> In Impala source code, I've encountered a place in
>>>> be/src/codegen/llvm-codegen.cc where DataLayout's pointer is passed to
>>>> PassManager::addPass method.
>>>>
>>>> module_pass_manager->add(new DataLayout(data_layout_str));
>>>> fn_pass_manager->add(new DataLayout(data_layout_str));
>>>>
>>>>
>>>> *Note*: addPass method is also changed to add in LLVM 3.7.
>>>>
>>>> I wanted to understand what exactly these two lines are doing. What
>>>> would be the impact if they are commented/removed? And if there is any
test
>>>> coverage for this?
>>>>
>>>> My rationale behind all these questions is to port Impala on ppc64le,
>>>> I'd to upgrade LLVM 3.3 to 3.7. And in LLVM 3.7, DataLayout is not
derived
>>>> from class Pass and hence these lines do not compile with LLVM 3.7.
So, I
>>>> need to either find equivalent of this or have to remove.
>>>>
>>>> I'll be grateful to you if you could provide some insights here.
>>>>
>>>> Thanks in advance,
>>>> Nishidha
>>>>
>>>> --
>>> You received this message because you are subscribed to the Google
Groups
>>> "Impala Dev" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
an
>>> email to impala-dev+unsubscribe@cloudera.org.
>>>
>>
>> --
>> You received this message because you are subscribed to the Google
Groups
>> "Impala Dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send
an
>> email to impala-dev+unsubscribe@cloudera.org.
>>
>
>
>
> --
> Henry Robinson
> Software Engineer
> Cloudera
> 415-994-6679
--
Todd Lipcon
Software Engineer, Cloudera
Re: Passing DataLayout to Module/Function Pass Manager of LLVM
Posted by Todd Lipcon <to...@cloudera.com>.
You might consider skipping 3.7 and going right to 3.8. It's in its second
release candidate and should be released in the next week or so.
-Todd
On Tue, Feb 23, 2016 at 3:52 PM, Tim Armstrong <ta...@cloudera.com>
wrote:
> Hi Nishidha,
> I'm going to spend some time on the LLVM 3.7 upgrade, likely next week,
> once we finish up work on our next release. If you have a patch you are
> able to share now it would be interesting to compare.
>
> It looks like I probably have a bit of work to get everything working
> correctly in LLVM 3.7 and to validate the performance.
>
> Cheers,
> TIm
>
> On Fri, Feb 19, 2016 at 2:36 AM, Nishidha Panpaliya <ni...@us.ibm.com>
> wrote:
>
>> Thanks Todd for this additional information. I'll definitely check this
>> once I'm done with porting Impala successfully.
>>
>> Thanks again.
>> Nishidha
>>
>> [image: Inactive hide details for Todd Lipcon ---02/19/2016 02:08:42
>> AM---On Thu, Feb 18, 2016 at 5:16 AM, Nishidha Panpaliya <nishidha]Todd
>> Lipcon ---02/19/2016 02:08:42 AM---On Thu, Feb 18, 2016 at 5:16 AM,
>> Nishidha Panpaliya <ni...@us.ibm.com> wrote:
>>
>> From: Todd Lipcon <to...@cloudera.com>
>> To: Nishidha Panpaliya/Austin/Contr/IBM@IBMUS
>> Cc: dev@impala.incubator.apache.org, nishidha panpaliya <
>> nishidha27@gmail.com>, Silvius Rus <sr...@cloudera.com>, Sudarshan
>> Jagadale/Austin/Contr/IBM@IBMUS
>> Date: 02/19/2016 02:08 AM
>>
>> Subject: Re: Passing DataLayout to Module/Function Pass Manager of LLVM
>> ------------------------------
>>
>>
>>
>> On Thu, Feb 18, 2016 at 5:16 AM, Nishidha Panpaliya <
>> *nishidha@us.ibm.com* <ni...@us.ibm.com>> wrote:
>>
>> Hi Todd,
>>
>> Thanks a lot. This really helped me.
>>
>> So, in case I can simply ignore module->setDataLayout(...) as it is
>> already set in it once. In my case, with LLVM 3.3, datalayout was added as
>> Pass to ModulePassManager and FunctionPassManager, which now is not needed,
>> as it is already set in the module object being used by these passmanagers.
>> Please confirm my understanding.
>>
>> Also, it seems in your module_builder.cc, you are continuing using
>> PassManagers which are made as legacy now in LLVM 3.7. I hope it is okay to
>> use those legacy PassManagers instead of new PassManagers in
>> IR/PassManager.h.
>>
>>
>>
>> I haven't followed this API change closely -- I didn't realize that
>> PassManagers are legacy now. It seems like our codegen is working, though :)
>>
>> One thing to be aware of is that some inlining heuristics or thresholds
>> changed with our upgrade. We had to do some patches to get the generated
>> code back to the original performance. So, make sure you benchmark
>> carefully and/or inspect the generated machine code for unwanted call
>> instructions. (in the Kudu code we have a -codegen_dump_mc flag to allow
>> dumping the asm, not sure if Impala has the same)
>>
>> -Todd
>>
>>
>> [image: Inactive hide details for Todd Lipcon ---02/17/2016 11:45:53
>> PM---Regarding the DataLayout question, here's how we handle it wi]Todd
>> Lipcon ---02/17/2016 11:45:53 PM---Regarding the DataLayout question,
>> here's how we handle it with LLVM 3.7 in Kudu:
>>
>> From: Todd Lipcon <*todd@cloudera.com* <to...@cloudera.com>>
>> To: *dev@impala.incubator.apache.org*
>> <de...@impala.incubator.apache.org>
>> Cc: Silvius Rus <*srus@cloudera.com* <sr...@cloudera.com>>, nishidha
>> panpaliya <*nishidha27@gmail.com* <ni...@gmail.com>>, Nishidha
>> Panpaliya/Austin/Contr/IBM@IBMUS, Sudarshan
>> Jagadale/Austin/Contr/IBM@IBMUS
>> Date: 02/17/2016 11:45 PM
>> Subject: Re: Passing DataLayout to Module/Function Pass Manager of
>> LLVM
>> ------------------------------
>>
>>
>>
>>
>> Regarding the DataLayout question, here's how we handle it with LLVM
>> 3.7 in Kudu:
>>
>>
>> *https://github.com/cloudera/kudu/blob/master/src/kudu/codegen/module_builder.cc#L267*
>> <https://github.com/cloudera/kudu/blob/master/src/kudu/codegen/module_builder.cc#L267>
>>
>> You can see the diff in this mega commit:
>>
>> *https://github.com/cloudera/kudu/commit/9806863e78107505a622b44112a897189d9b3c24*
>> <https://github.com/cloudera/kudu/commit/9806863e78107505a622b44112a897189d9b3c24>
>>
>> On Wed, Feb 17, 2016 at 10:08 AM, Henry Robinson <*henry@cloudera.com*
>> <he...@cloudera.com>> wrote:
>> > (Moving this conversation to *dev@impala.incubator.apache.org*
>> <de...@impala.incubator.apache.org>,
>> > *impala-dev@cloudera.org* <im...@cloudera.org> to bcc:)
>> >
>> > On 17 February 2016 at 09:13, Silvius Rus <*srus@cloudera.com*
>> <sr...@cloudera.com>> wrote:
>> >
>> >> Hey Nishidha,
>> >>
>> >> Would you be interested in contributing the changes you've made so
>> far to
>> >> the Impala ASF project? It's easier to get help in context if you
>> post a
>> >> patch for review. Upgrading to LLVM 3.7 would be beneficial to
>> the project
>> >> in general, so I think others will jump in to help.
>> >>
>> >> *https://github.com/cloudera/Impala/wiki/Contributing-to-Impala*
>> <https://github.com/cloudera/Impala/wiki/Contributing-to-Impala>
>> >>
>> >> Silvius
>> >>
>> >> On Wed, Feb 17, 2016 at 5:00 AM, nishidha panpaliya <
>> *nishidha27@gmail.com* <ni...@gmail.com>>
>> >> wrote:
>> >>
>> >>> Alright Tim. Thanks for the reply.
>> >>>
>> >>> Regards,
>> >>> Nishidha
>> >>>
>> >>>
>> >>> On Tuesday, 16 February 2016 19:16:34 UTC+5:30, nishidha
>> panpaliya wrote:
>> >>>>
>> >>>> Hello,
>> >>>>
>> >>>> In Impala source code, I've encountered a place in
>> >>>> be/src/codegen/llvm-codegen.cc where DataLayout's pointer is
>> passed to
>> >>>> PassManager::addPass method.
>> >>>>
>> >>>> module_pass_manager->add(new DataLayout(data_layout_str));
>> >>>> fn_pass_manager->add(new DataLayout(data_layout_str));
>> >>>>
>> >>>>
>> >>>> *Note*: addPass method is also changed to add in LLVM 3.7.
>> >>>>
>> >>>> I wanted to understand what exactly these two lines are doing.
>> What
>> >>>> would be the impact if they are commented/removed? And if there
>> is any test
>> >>>> coverage for this?
>> >>>>
>> >>>> My rationale behind all these questions is to port Impala on
>> ppc64le,
>> >>>> I'd to upgrade LLVM 3.3 to 3.7. And in LLVM 3.7, DataLayout is
>> not derived
>> >>>> from class Pass and hence these lines do not compile with LLVM
>> 3.7. So, I
>> >>>> need to either find equivalent of this or have to remove.
>> >>>>
>> >>>> I'll be grateful to you if you could provide some insights here.
>> >>>>
>> >>>> Thanks in advance,
>> >>>> Nishidha
>> >>>>
>> >>>> --
>> >>> You received this message because you are subscribed to the
>> Google Groups
>> >>> "Impala Dev" group.
>> >>> To unsubscribe from this group and stop receiving emails from it,
>> send an
>> >>> email to *impala-dev+unsubscribe@cloudera.org*
>> <im...@cloudera.org>.
>> >>>
>> >>
>> >> --
>> >> You received this message because you are subscribed to the Google
>> Groups
>> >> "Impala Dev" group.
>> >> To unsubscribe from this group and stop receiving emails from it,
>> send an
>> >> email to *impala-dev+unsubscribe@cloudera.org*
>> <im...@cloudera.org>.
>> >>
>> >
>> >
>> >
>> > --
>> > Henry Robinson
>> > Software Engineer
>> > Cloudera
>> > *415-994-6679* <415-994-6679>
>>
>>
>>
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>
>>
>>
>>
>>
>>
>>
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>
>>
>
--
Todd Lipcon
Software Engineer, Cloudera
Re: Passing DataLayout to Module/Function Pass Manager of LLVM
Posted by Tim Armstrong <ta...@cloudera.com>.
Hi Nishidha,
I'm going to spend some time on the LLVM 3.7 upgrade, likely next week,
once we finish up work on our next release. If you have a patch you are
able to share now it would be interesting to compare.
It looks like I probably have a bit of work to get everything working
correctly in LLVM 3.7 and to validate the performance.
Cheers,
TIm
On Fri, Feb 19, 2016 at 2:36 AM, Nishidha Panpaliya <ni...@us.ibm.com>
wrote:
> Thanks Todd for this additional information. I'll definitely check this
> once I'm done with porting Impala successfully.
>
> Thanks again.
> Nishidha
>
> [image: Inactive hide details for Todd Lipcon ---02/19/2016 02:08:42
> AM---On Thu, Feb 18, 2016 at 5:16 AM, Nishidha Panpaliya <nishidha]Todd
> Lipcon ---02/19/2016 02:08:42 AM---On Thu, Feb 18, 2016 at 5:16 AM,
> Nishidha Panpaliya <ni...@us.ibm.com> wrote:
>
> From: Todd Lipcon <to...@cloudera.com>
> To: Nishidha Panpaliya/Austin/Contr/IBM@IBMUS
> Cc: dev@impala.incubator.apache.org, nishidha panpaliya <
> nishidha27@gmail.com>, Silvius Rus <sr...@cloudera.com>, Sudarshan
> Jagadale/Austin/Contr/IBM@IBMUS
> Date: 02/19/2016 02:08 AM
>
> Subject: Re: Passing DataLayout to Module/Function Pass Manager of LLVM
> ------------------------------
>
>
>
> On Thu, Feb 18, 2016 at 5:16 AM, Nishidha Panpaliya <*nishidha@us.ibm.com*
> <ni...@us.ibm.com>> wrote:
>
> Hi Todd,
>
> Thanks a lot. This really helped me.
>
> So, in case I can simply ignore module->setDataLayout(...) as it is
> already set in it once. In my case, with LLVM 3.3, datalayout was added as
> Pass to ModulePassManager and FunctionPassManager, which now is not needed,
> as it is already set in the module object being used by these passmanagers.
> Please confirm my understanding.
>
> Also, it seems in your module_builder.cc, you are continuing using
> PassManagers which are made as legacy now in LLVM 3.7. I hope it is okay to
> use those legacy PassManagers instead of new PassManagers in
> IR/PassManager.h.
>
>
>
> I haven't followed this API change closely -- I didn't realize that
> PassManagers are legacy now. It seems like our codegen is working, though :)
>
> One thing to be aware of is that some inlining heuristics or thresholds
> changed with our upgrade. We had to do some patches to get the generated
> code back to the original performance. So, make sure you benchmark
> carefully and/or inspect the generated machine code for unwanted call
> instructions. (in the Kudu code we have a -codegen_dump_mc flag to allow
> dumping the asm, not sure if Impala has the same)
>
> -Todd
>
>
> [image: Inactive hide details for Todd Lipcon ---02/17/2016 11:45:53
> PM---Regarding the DataLayout question, here's how we handle it wi]Todd
> Lipcon ---02/17/2016 11:45:53 PM---Regarding the DataLayout question,
> here's how we handle it with LLVM 3.7 in Kudu:
>
> From: Todd Lipcon <*todd@cloudera.com* <to...@cloudera.com>>
> To: *dev@impala.incubator.apache.org* <de...@impala.incubator.apache.org>
> Cc: Silvius Rus <*srus@cloudera.com* <sr...@cloudera.com>>, nishidha
> panpaliya <*nishidha27@gmail.com* <ni...@gmail.com>>, Nishidha
> Panpaliya/Austin/Contr/IBM@IBMUS, Sudarshan
> Jagadale/Austin/Contr/IBM@IBMUS
> Date: 02/17/2016 11:45 PM
> Subject: Re: Passing DataLayout to Module/Function Pass Manager of LLVM
> ------------------------------
>
>
>
>
> Regarding the DataLayout question, here's how we handle it with LLVM
> 3.7 in Kudu:
>
>
> *https://github.com/cloudera/kudu/blob/master/src/kudu/codegen/module_builder.cc#L267*
> <https://github.com/cloudera/kudu/blob/master/src/kudu/codegen/module_builder.cc#L267>
>
> You can see the diff in this mega commit:
>
> *https://github.com/cloudera/kudu/commit/9806863e78107505a622b44112a897189d9b3c24*
> <https://github.com/cloudera/kudu/commit/9806863e78107505a622b44112a897189d9b3c24>
>
> On Wed, Feb 17, 2016 at 10:08 AM, Henry Robinson <*henry@cloudera.com*
> <he...@cloudera.com>> wrote:
> > (Moving this conversation to *dev@impala.incubator.apache.org*
> <de...@impala.incubator.apache.org>,
> > *impala-dev@cloudera.org* <im...@cloudera.org> to bcc:)
> >
> > On 17 February 2016 at 09:13, Silvius Rus <*srus@cloudera.com*
> <sr...@cloudera.com>> wrote:
> >
> >> Hey Nishidha,
> >>
> >> Would you be interested in contributing the changes you've made so
> far to
> >> the Impala ASF project? It's easier to get help in context if you
> post a
> >> patch for review. Upgrading to LLVM 3.7 would be beneficial to the
> project
> >> in general, so I think others will jump in to help.
> >>
> >> *https://github.com/cloudera/Impala/wiki/Contributing-to-Impala*
> <https://github.com/cloudera/Impala/wiki/Contributing-to-Impala>
> >>
> >> Silvius
> >>
> >> On Wed, Feb 17, 2016 at 5:00 AM, nishidha panpaliya <
> *nishidha27@gmail.com* <ni...@gmail.com>>
> >> wrote:
> >>
> >>> Alright Tim. Thanks for the reply.
> >>>
> >>> Regards,
> >>> Nishidha
> >>>
> >>>
> >>> On Tuesday, 16 February 2016 19:16:34 UTC+5:30, nishidha panpaliya
> wrote:
> >>>>
> >>>> Hello,
> >>>>
> >>>> In Impala source code, I've encountered a place in
> >>>> be/src/codegen/llvm-codegen.cc where DataLayout's pointer is
> passed to
> >>>> PassManager::addPass method.
> >>>>
> >>>> module_pass_manager->add(new DataLayout(data_layout_str));
> >>>> fn_pass_manager->add(new DataLayout(data_layout_str));
> >>>>
> >>>>
> >>>> *Note*: addPass method is also changed to add in LLVM 3.7.
> >>>>
> >>>> I wanted to understand what exactly these two lines are doing.
> What
> >>>> would be the impact if they are commented/removed? And if there
> is any test
> >>>> coverage for this?
> >>>>
> >>>> My rationale behind all these questions is to port Impala on
> ppc64le,
> >>>> I'd to upgrade LLVM 3.3 to 3.7. And in LLVM 3.7, DataLayout is
> not derived
> >>>> from class Pass and hence these lines do not compile with LLVM
> 3.7. So, I
> >>>> need to either find equivalent of this or have to remove.
> >>>>
> >>>> I'll be grateful to you if you could provide some insights here.
> >>>>
> >>>> Thanks in advance,
> >>>> Nishidha
> >>>>
> >>>> --
> >>> You received this message because you are subscribed to the Google
> Groups
> >>> "Impala Dev" group.
> >>> To unsubscribe from this group and stop receiving emails from it,
> send an
> >>> email to *impala-dev+unsubscribe@cloudera.org*
> <im...@cloudera.org>.
> >>>
> >>
> >> --
> >> You received this message because you are subscribed to the Google
> Groups
> >> "Impala Dev" group.
> >> To unsubscribe from this group and stop receiving emails from it,
> send an
> >> email to *impala-dev+unsubscribe@cloudera.org*
> <im...@cloudera.org>.
> >>
> >
> >
> >
> > --
> > Henry Robinson
> > Software Engineer
> > Cloudera
> > *415-994-6679* <415-994-6679>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>
>
>
>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>
>
Re: Passing DataLayout to Module/Function Pass Manager of LLVM
Posted by Nishidha Panpaliya <ni...@us.ibm.com>.
Thanks Todd for this additional information. I'll definitely check this
once I'm done with porting Impala successfully.
Thanks again.
Nishidha
From: Todd Lipcon <to...@cloudera.com>
To: Nishidha Panpaliya/Austin/Contr/IBM@IBMUS
Cc: dev@impala.incubator.apache.org, nishidha panpaliya
<ni...@gmail.com>, Silvius Rus <sr...@cloudera.com>,
Sudarshan Jagadale/Austin/Contr/IBM@IBMUS
Date: 02/19/2016 02:08 AM
Subject: Re: Passing DataLayout to Module/Function Pass Manager of LLVM
On Thu, Feb 18, 2016 at 5:16 AM, Nishidha Panpaliya <ni...@us.ibm.com>
wrote:
Hi Todd,
Thanks a lot. This really helped me.
So, in case I can simply ignore module->setDataLayout(...) as it is
already set in it once. In my case, with LLVM 3.3, datalayout was added
as Pass to ModulePassManager and FunctionPassManager, which now is not
needed, as it is already set in the module object being used by these
passmanagers.
Please confirm my understanding.
Also, it seems in your module_builder.cc, you are continuing using
PassManagers which are made as legacy now in LLVM 3.7. I hope it is okay
to use those legacy PassManagers instead of new PassManagers in
IR/PassManager.h.
I haven't followed this API change closely -- I didn't realize that
PassManagers are legacy now. It seems like our codegen is working,
though :)
One thing to be aware of is that some inlining heuristics or thresholds
changed with our upgrade. We had to do some patches to get the generated
code back to the original performance. So, make sure you benchmark
carefully and/or inspect the generated machine code for unwanted call
instructions. (in the Kudu code we have a -codegen_dump_mc flag to allow
dumping the asm, not sure if Impala has the same)
-Todd
Inactive hide details for Todd Lipcon ---02/17/2016 11:45:53
PM---Regarding the DataLayout question, here's how we handle it wiTodd
Lipcon ---02/17/2016 11:45:53 PM---Regarding the DataLayout question,
here's how we handle it with LLVM 3.7 in Kudu:
From: Todd Lipcon <to...@cloudera.com>
To: dev@impala.incubator.apache.org
Cc: Silvius Rus <sr...@cloudera.com>, nishidha panpaliya <
nishidha27@gmail.com>, Nishidha Panpaliya/Austin/Contr/IBM@IBMUS,
Sudarshan Jagadale/Austin/Contr/IBM@IBMUS
Date: 02/17/2016 11:45 PM
Subject: Re: Passing DataLayout to Module/Function Pass Manager of LLVM
Regarding the DataLayout question, here's how we handle it with LLVM
3.7 in Kudu:
https://github.com/cloudera/kudu/blob/master/src/kudu/codegen/module_builder.cc#L267
You can see the diff in this mega commit:
https://github.com/cloudera/kudu/commit/9806863e78107505a622b44112a897189d9b3c24
On Wed, Feb 17, 2016 at 10:08 AM, Henry Robinson <he...@cloudera.com>
wrote:
> (Moving this conversation to dev@impala.incubator.apache.org,
> impala-dev@cloudera.org to bcc:)
>
> On 17 February 2016 at 09:13, Silvius Rus <sr...@cloudera.com> wrote:
>
>> Hey Nishidha,
>>
>> Would you be interested in contributing the changes you've made so far
to
>> the Impala ASF project? It's easier to get help in context if you
post a
>> patch for review. Upgrading to LLVM 3.7 would be beneficial to the
project
>> in general, so I think others will jump in to help.
>>
>> https://github.com/cloudera/Impala/wiki/Contributing-to-Impala
>>
>> Silvius
>>
>> On Wed, Feb 17, 2016 at 5:00 AM, nishidha panpaliya <
nishidha27@gmail.com>
>> wrote:
>>
>>> Alright Tim. Thanks for the reply.
>>>
>>> Regards,
>>> Nishidha
>>>
>>>
>>> On Tuesday, 16 February 2016 19:16:34 UTC+5:30, nishidha panpaliya
wrote:
>>>>
>>>> Hello,
>>>>
>>>> In Impala source code, I've encountered a place in
>>>> be/src/codegen/llvm-codegen.cc where DataLayout's pointer is passed
to
>>>> PassManager::addPass method.
>>>>
>>>> module_pass_manager->add(new DataLayout(data_layout_str));
>>>> fn_pass_manager->add(new DataLayout(data_layout_str));
>>>>
>>>>
>>>> *Note*: addPass method is also changed to add in LLVM 3.7.
>>>>
>>>> I wanted to understand what exactly these two lines are doing. What
>>>> would be the impact if they are commented/removed? And if there is
any test
>>>> coverage for this?
>>>>
>>>> My rationale behind all these questions is to port Impala on
ppc64le,
>>>> I'd to upgrade LLVM 3.3 to 3.7. And in LLVM 3.7, DataLayout is not
derived
>>>> from class Pass and hence these lines do not compile with LLVM 3.7.
So, I
>>>> need to either find equivalent of this or have to remove.
>>>>
>>>> I'll be grateful to you if you could provide some insights here.
>>>>
>>>> Thanks in advance,
>>>> Nishidha
>>>>
>>>> --
>>> You received this message because you are subscribed to the Google
Groups
>>> "Impala Dev" group.
>>> To unsubscribe from this group and stop receiving emails from it,
send an
>>> email to impala-dev+unsubscribe@cloudera.org.
>>>
>>
>> --
>> You received this message because you are subscribed to the Google
Groups
>> "Impala Dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send
an
>> email to impala-dev+unsubscribe@cloudera.org.
>>
>
>
>
> --
> Henry Robinson
> Software Engineer
> Cloudera
> 415-994-6679
--
Todd Lipcon
Software Engineer, Cloudera
--
Todd Lipcon
Software Engineer, Cloudera
Re: Passing DataLayout to Module/Function Pass Manager of LLVM
Posted by Todd Lipcon <to...@cloudera.com>.
On Thu, Feb 18, 2016 at 5:16 AM, Nishidha Panpaliya <ni...@us.ibm.com>
wrote:
> Hi Todd,
>
> Thanks a lot. This really helped me.
>
> So, in case I can simply ignore module->setDataLayout(...) as it is
> already set in it once. In my case, with LLVM 3.3, datalayout was added as
> Pass to ModulePassManager and FunctionPassManager, which now is not needed,
> as it is already set in the module object being used by these passmanagers.
> Please confirm my understanding.
>
> Also, it seems in your module_builder.cc, you are continuing using
> PassManagers which are made as legacy now in LLVM 3.7. I hope it is okay to
> use those legacy PassManagers instead of new PassManagers in
> IR/PassManager.h.
>
I haven't followed this API change closely -- I didn't realize that
PassManagers are legacy now. It seems like our codegen is working, though :)
One thing to be aware of is that some inlining heuristics or thresholds
changed with our upgrade. We had to do some patches to get the generated
code back to the original performance. So, make sure you benchmark
carefully and/or inspect the generated machine code for unwanted call
instructions. (in the Kudu code we have a -codegen_dump_mc flag to allow
dumping the asm, not sure if Impala has the same)
-Todd
> [image: Inactive hide details for Todd Lipcon ---02/17/2016 11:45:53
> PM---Regarding the DataLayout question, here's how we handle it wi]Todd
> Lipcon ---02/17/2016 11:45:53 PM---Regarding the DataLayout question,
> here's how we handle it with LLVM 3.7 in Kudu:
>
> From: Todd Lipcon <to...@cloudera.com>
> To: dev@impala.incubator.apache.org
> Cc: Silvius Rus <sr...@cloudera.com>, nishidha panpaliya <
> nishidha27@gmail.com>, Nishidha Panpaliya/Austin/Contr/IBM@IBMUS,
> Sudarshan Jagadale/Austin/Contr/IBM@IBMUS
> Date: 02/17/2016 11:45 PM
> Subject: Re: Passing DataLayout to Module/Function Pass Manager of LLVM
> ------------------------------
>
>
>
> Regarding the DataLayout question, here's how we handle it with LLVM
> 3.7 in Kudu:
>
>
> https://github.com/cloudera/kudu/blob/master/src/kudu/codegen/module_builder.cc#L267
>
> You can see the diff in this mega commit:
>
> https://github.com/cloudera/kudu/commit/9806863e78107505a622b44112a897189d9b3c24
>
> On Wed, Feb 17, 2016 at 10:08 AM, Henry Robinson <he...@cloudera.com>
> wrote:
> > (Moving this conversation to dev@impala.incubator.apache.org,
> > impala-dev@cloudera.org to bcc:)
> >
> > On 17 February 2016 at 09:13, Silvius Rus <sr...@cloudera.com> wrote:
> >
> >> Hey Nishidha,
> >>
> >> Would you be interested in contributing the changes you've made so far
> to
> >> the Impala ASF project? It's easier to get help in context if you post
> a
> >> patch for review. Upgrading to LLVM 3.7 would be beneficial to the
> project
> >> in general, so I think others will jump in to help.
> >>
> >> https://github.com/cloudera/Impala/wiki/Contributing-to-Impala
> >>
> >> Silvius
> >>
> >> On Wed, Feb 17, 2016 at 5:00 AM, nishidha panpaliya <
> nishidha27@gmail.com>
> >> wrote:
> >>
> >>> Alright Tim. Thanks for the reply.
> >>>
> >>> Regards,
> >>> Nishidha
> >>>
> >>>
> >>> On Tuesday, 16 February 2016 19:16:34 UTC+5:30, nishidha panpaliya
> wrote:
> >>>>
> >>>> Hello,
> >>>>
> >>>> In Impala source code, I've encountered a place in
> >>>> be/src/codegen/llvm-codegen.cc where DataLayout's pointer is passed to
> >>>> PassManager::addPass method.
> >>>>
> >>>> module_pass_manager->add(new DataLayout(data_layout_str));
> >>>> fn_pass_manager->add(new DataLayout(data_layout_str));
> >>>>
> >>>>
> >>>> *Note*: addPass method is also changed to add in LLVM 3.7.
> >>>>
> >>>> I wanted to understand what exactly these two lines are doing. What
> >>>> would be the impact if they are commented/removed? And if there is
> any test
> >>>> coverage for this?
> >>>>
> >>>> My rationale behind all these questions is to port Impala on ppc64le,
> >>>> I'd to upgrade LLVM 3.3 to 3.7. And in LLVM 3.7, DataLayout is not
> derived
> >>>> from class Pass and hence these lines do not compile with LLVM 3.7.
> So, I
> >>>> need to either find equivalent of this or have to remove.
> >>>>
> >>>> I'll be grateful to you if you could provide some insights here.
> >>>>
> >>>> Thanks in advance,
> >>>> Nishidha
> >>>>
> >>>> --
> >>> You received this message because you are subscribed to the Google
> Groups
> >>> "Impala Dev" group.
> >>> To unsubscribe from this group and stop receiving emails from it, send
> an
> >>> email to impala-dev+unsubscribe@cloudera.org.
> >>>
> >>
> >> --
> >> You received this message because you are subscribed to the Google
> Groups
> >> "Impala Dev" group.
> >> To unsubscribe from this group and stop receiving emails from it, send
> an
> >> email to impala-dev+unsubscribe@cloudera.org.
> >>
> >
> >
> >
> > --
> > Henry Robinson
> > Software Engineer
> > Cloudera
> > 415-994-6679
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>
>
>
>
--
Todd Lipcon
Software Engineer, Cloudera
Re: Passing DataLayout to Module/Function Pass Manager of LLVM
Posted by Todd Lipcon <to...@cloudera.com>.
Regarding the DataLayout question, here's how we handle it with LLVM
3.7 in Kudu:
https://github.com/cloudera/kudu/blob/master/src/kudu/codegen/module_builder.cc#L267
You can see the diff in this mega commit:
https://github.com/cloudera/kudu/commit/9806863e78107505a622b44112a897189d9b3c24
On Wed, Feb 17, 2016 at 10:08 AM, Henry Robinson <he...@cloudera.com> wrote:
> (Moving this conversation to dev@impala.incubator.apache.org,
> impala-dev@cloudera.org to bcc:)
>
> On 17 February 2016 at 09:13, Silvius Rus <sr...@cloudera.com> wrote:
>
>> Hey Nishidha,
>>
>> Would you be interested in contributing the changes you've made so far to
>> the Impala ASF project? It's easier to get help in context if you post a
>> patch for review. Upgrading to LLVM 3.7 would be beneficial to the project
>> in general, so I think others will jump in to help.
>>
>> https://github.com/cloudera/Impala/wiki/Contributing-to-Impala
>>
>> Silvius
>>
>> On Wed, Feb 17, 2016 at 5:00 AM, nishidha panpaliya <ni...@gmail.com>
>> wrote:
>>
>>> Alright Tim. Thanks for the reply.
>>>
>>> Regards,
>>> Nishidha
>>>
>>>
>>> On Tuesday, 16 February 2016 19:16:34 UTC+5:30, nishidha panpaliya wrote:
>>>>
>>>> Hello,
>>>>
>>>> In Impala source code, I've encountered a place in
>>>> be/src/codegen/llvm-codegen.cc where DataLayout's pointer is passed to
>>>> PassManager::addPass method.
>>>>
>>>> module_pass_manager->add(new DataLayout(data_layout_str));
>>>> fn_pass_manager->add(new DataLayout(data_layout_str));
>>>>
>>>>
>>>> *Note*: addPass method is also changed to add in LLVM 3.7.
>>>>
>>>> I wanted to understand what exactly these two lines are doing. What
>>>> would be the impact if they are commented/removed? And if there is any test
>>>> coverage for this?
>>>>
>>>> My rationale behind all these questions is to port Impala on ppc64le,
>>>> I'd to upgrade LLVM 3.3 to 3.7. And in LLVM 3.7, DataLayout is not derived
>>>> from class Pass and hence these lines do not compile with LLVM 3.7. So, I
>>>> need to either find equivalent of this or have to remove.
>>>>
>>>> I'll be grateful to you if you could provide some insights here.
>>>>
>>>> Thanks in advance,
>>>> Nishidha
>>>>
>>>> --
>>> You received this message because you are subscribed to the Google Groups
>>> "Impala Dev" group.
>>> To unsubscribe from this group and stop receiving emails from it, send an
>>> email to impala-dev+unsubscribe@cloudera.org.
>>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Impala Dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to impala-dev+unsubscribe@cloudera.org.
>>
>
>
>
> --
> Henry Robinson
> Software Engineer
> Cloudera
> 415-994-6679
--
Todd Lipcon
Software Engineer, Cloudera