Hi Anton and JF, Thanks for your review. After reading the source code more carefully, I have come up with a different way fix this issue. We can simply resolve this issue by adding ARMII::MO_PLT flags with MachineInstrBuilder in FastISel pass (without failing back to DAG lowering). The new patch is attached, and the test case is not changed. Sorry for your inconvenience. Please have a look. Thanks for your help. Sincerely, Logan On Wed, Aug 21, 2013 at 10:52 PM, JF Bastien <jfb at google.com> wrote:> lgtm > > > On Wed, Aug 21, 2013 at 3:18 AM, Anton Korobeynikov < > anton at korobeynikov.info> wrote: > >> LGTM >> >> On Wed, Aug 21, 2013 at 1:51 PM, Logan Chien <tzuhsiang.chien at gmail.com> >> wrote: >> > Hi, >> > >> > I have created a workaround to deal with the PIC function call. With >> this >> > patch, the FastISel will switch back to DAG lowering mechanism if (1) >> there >> > is a function call in the basic block and (2) the relocation model is >> PIC. >> > Please have a look. Hoping the patch will help. >> > >> > Sincerely, >> > Logan >> > >> > >> > On Wed, Aug 21, 2013 at 10:17 AM, Gordon Keiser <gkeiser at arxan.com> >> wrote: >> >> >> >> Cool, I'll file a bug tomorrow at work and add you to the CC list. >> >> >> >> Thanks! >> >> Gordon Keiser >> >> Software Development Engineer >> >> Arxan Technologies >> >> gkeiser at arxan.com www.arxan.com >> >> Protecting the App EconomyT >> >> >> >> > -----Original Message----- >> >> > From: Eric Christopher [mailto:echristo at gmail.com] >> >> > Sent: Tuesday, August 20, 2013 9:47 PM >> >> > To: Gordon Keiser >> >> > Cc: llvmdev at cs.uiuc.edu; JF Bastien >> >> > Subject: Re: [LLVMdev] Broken PLT on ARM from R183966 >> >> > >> >> > Filing a bug would be a good start, go ahead and cc me and >> >> > jfb at google.com. >> >> > >> >> > Thanks! >> >> > >> >> > -eric >> >> > >> >> > On Tue, Aug 20, 2013 at 6:10 PM, Gordon Keiser <gkeiser at arxan.com> >> >> > wrote: >> >> > > For ARM targets on linux, revision 183966 made Fast ISel default. >> >> > > Unfortunately, Fast ISel is broken in terms of applying the >> >> > > ARMII::MO_PLT flags to calls in PIC mode (at least when emitting >> >> > > assembly); it never does this. The normal ISel pass handles this >> >> > > situation correctly so a temporary local change to disable FastISel >> >> > > for linux / NaCl targets is working for me right now. >> >> > > >> >> > > >> >> > > >> >> > > I'm not very familiar with the ISel passes. I'm guessing the >> correct >> >> > > thing to do here would be to apply the attribute correctly in >> FastISel >> >> > > so it works, but I'm kind of unfamiliar with this part of the code >> and >> >> > > won't have time to dig into it right now, although I'm willing to >> do >> >> > > it if someone points me to the right area of code. >> >> > > >> >> > > >> >> > > >> >> > > In the meantime, would it be worthwhile to submit in a >> modification to >> >> > > disable FastISel for non-darwin targets, or is it likely to be >> fixed >> >> > > quickly? >> >> > > >> >> > > >> >> > > >> >> > > -Gordon >> >> > > >> >> > > >> >> > > _______________________________________________ >> >> > > LLVM Developers mailing list >> >> > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> >> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >> > > >> >> >> >> _______________________________________________ >> >> LLVM Developers mailing list >> >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > >> > >> > >> > _______________________________________________ >> > LLVM Developers mailing list >> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > >> >> >> >> -- >> With best regards, Anton Korobeynikov >> Faculty of Mathematics and Mechanics, Saint Petersburg State University >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130821/3d4bce48/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Fix-ARM-FastISel-PIC-function-call.patch Type: application/octet-stream Size: 2331 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130821/3d4bce48/attachment.obj>
I'm not very familiar with relocations but your fix looks the same as ARMTargetLowering::LowerCall, so from that perspective it lgtm (but I may be missing something). On Wed, Aug 21, 2013 at 8:04 AM, Logan Chien <tzuhsiang.chien at gmail.com>wrote:> Hi Anton and JF, > > Thanks for your review. After reading the source code more carefully, I > have come up with a different way fix this issue. We can simply resolve > this issue by adding ARMII::MO_PLT flags with MachineInstrBuilder in > FastISel pass (without failing back to DAG lowering). > > The new patch is attached, and the test case is not changed. Sorry for > your inconvenience. Please have a look. Thanks for your help. > > Sincerely, > Logan > > > On Wed, Aug 21, 2013 at 10:52 PM, JF Bastien <jfb at google.com> wrote: > >> lgtm >> >> >> On Wed, Aug 21, 2013 at 3:18 AM, Anton Korobeynikov < >> anton at korobeynikov.info> wrote: >> >>> LGTM >>> >>> On Wed, Aug 21, 2013 at 1:51 PM, Logan Chien <tzuhsiang.chien at gmail.com> >>> wrote: >>> > Hi, >>> > >>> > I have created a workaround to deal with the PIC function call. >>> With this >>> > patch, the FastISel will switch back to DAG lowering mechanism if (1) >>> there >>> > is a function call in the basic block and (2) the relocation model is >>> PIC. >>> > Please have a look. Hoping the patch will help. >>> > >>> > Sincerely, >>> > Logan >>> > >>> > >>> > On Wed, Aug 21, 2013 at 10:17 AM, Gordon Keiser <gkeiser at arxan.com> >>> wrote: >>> >> >>> >> Cool, I'll file a bug tomorrow at work and add you to the CC list. >>> >> >>> >> Thanks! >>> >> Gordon Keiser >>> >> Software Development Engineer >>> >> Arxan Technologies >>> >> gkeiser at arxan.com www.arxan.com >>> >> Protecting the App EconomyT >>> >> >>> >> > -----Original Message----- >>> >> > From: Eric Christopher [mailto:echristo at gmail.com] >>> >> > Sent: Tuesday, August 20, 2013 9:47 PM >>> >> > To: Gordon Keiser >>> >> > Cc: llvmdev at cs.uiuc.edu; JF Bastien >>> >> > Subject: Re: [LLVMdev] Broken PLT on ARM from R183966 >>> >> > >>> >> > Filing a bug would be a good start, go ahead and cc me and >>> >> > jfb at google.com. >>> >> > >>> >> > Thanks! >>> >> > >>> >> > -eric >>> >> > >>> >> > On Tue, Aug 20, 2013 at 6:10 PM, Gordon Keiser <gkeiser at arxan.com> >>> >> > wrote: >>> >> > > For ARM targets on linux, revision 183966 made Fast ISel default. >>> >> > > Unfortunately, Fast ISel is broken in terms of applying the >>> >> > > ARMII::MO_PLT flags to calls in PIC mode (at least when emitting >>> >> > > assembly); it never does this. The normal ISel pass handles this >>> >> > > situation correctly so a temporary local change to disable >>> FastISel >>> >> > > for linux / NaCl targets is working for me right now. >>> >> > > >>> >> > > >>> >> > > >>> >> > > I'm not very familiar with the ISel passes. I'm guessing the >>> correct >>> >> > > thing to do here would be to apply the attribute correctly in >>> FastISel >>> >> > > so it works, but I'm kind of unfamiliar with this part of the >>> code and >>> >> > > won't have time to dig into it right now, although I'm willing to >>> do >>> >> > > it if someone points me to the right area of code. >>> >> > > >>> >> > > >>> >> > > >>> >> > > In the meantime, would it be worthwhile to submit in a >>> modification to >>> >> > > disable FastISel for non-darwin targets, or is it likely to be >>> fixed >>> >> > > quickly? >>> >> > > >>> >> > > >>> >> > > >>> >> > > -Gordon >>> >> > > >>> >> > > >>> >> > > _______________________________________________ >>> >> > > LLVM Developers mailing list >>> >> > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> >> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >> > > >>> >> >>> >> _______________________________________________ >>> >> LLVM Developers mailing list >>> >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> > >>> > >>> > >>> > _______________________________________________ >>> > LLVM Developers mailing list >>> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> > >>> >>> >>> >>> -- >>> With best regards, Anton Korobeynikov >>> Faculty of Mathematics and Mechanics, Saint Petersburg State University >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130821/9cbb6104/attachment.html>
That change seems to fix things here. Thanks! -Gordon From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On Behalf Of JF Bastien Sent: Wednesday, August 21, 2013 12:53 PM To: Logan Chien Cc: llvmdev at cs.uiuc.edu Subject: Re: [LLVMdev] Broken PLT on ARM from R183966 I'm not very familiar with relocations but your fix looks the same as ARMTargetLowering::LowerCall, so from that perspective it lgtm (but I may be missing something). On Wed, Aug 21, 2013 at 8:04 AM, Logan Chien <tzuhsiang.chien at gmail.com<mailto:tzuhsiang.chien at gmail.com>> wrote: Hi Anton and JF, Thanks for your review. After reading the source code more carefully, I have come up with a different way fix this issue. We can simply resolve this issue by adding ARMII::MO_PLT flags with MachineInstrBuilder in FastISel pass (without failing back to DAG lowering). The new patch is attached, and the test case is not changed. Sorry for your inconvenience. Please have a look. Thanks for your help. Sincerely, Logan On Wed, Aug 21, 2013 at 10:52 PM, JF Bastien <jfb at google.com<mailto:jfb at google.com>> wrote: lgtm On Wed, Aug 21, 2013 at 3:18 AM, Anton Korobeynikov <anton at korobeynikov.info<mailto:anton at korobeynikov.info>> wrote: LGTM On Wed, Aug 21, 2013 at 1:51 PM, Logan Chien <tzuhsiang.chien at gmail.com<mailto:tzuhsiang.chien at gmail.com>> wrote:> Hi, > > I have created a workaround to deal with the PIC function call. With this > patch, the FastISel will switch back to DAG lowering mechanism if (1) there > is a function call in the basic block and (2) the relocation model is PIC. > Please have a look. Hoping the patch will help. > > Sincerely, > Logan > > > On Wed, Aug 21, 2013 at 10:17 AM, Gordon Keiser <gkeiser at arxan.com<mailto:gkeiser at arxan.com>> wrote: >> >> Cool, I'll file a bug tomorrow at work and add you to the CC list. >> >> Thanks! >> Gordon Keiser >> Software Development Engineer >> Arxan Technologies >> gkeiser at arxan.com<mailto:gkeiser at arxan.com> www.arxan.com<http://www.arxan.com> >> Protecting the App EconomyT >> >> > -----Original Message----- >> > From: Eric Christopher [mailto:echristo at gmail.com<mailto:echristo at gmail.com>] >> > Sent: Tuesday, August 20, 2013 9:47 PM >> > To: Gordon Keiser >> > Cc: llvmdev at cs.uiuc.edu<mailto:llvmdev at cs.uiuc.edu>; JF Bastien >> > Subject: Re: [LLVMdev] Broken PLT on ARM from R183966 >> > >> > Filing a bug would be a good start, go ahead and cc me and >> > jfb at google.com<mailto:jfb at google.com>. >> > >> > Thanks! >> > >> > -eric >> > >> > On Tue, Aug 20, 2013 at 6:10 PM, Gordon Keiser <gkeiser at arxan.com<mailto:gkeiser at arxan.com>> >> > wrote: >> > > For ARM targets on linux, revision 183966 made Fast ISel default. >> > > Unfortunately, Fast ISel is broken in terms of applying the >> > > ARMII::MO_PLT flags to calls in PIC mode (at least when emitting >> > > assembly); it never does this. The normal ISel pass handles this >> > > situation correctly so a temporary local change to disable FastISel >> > > for linux / NaCl targets is working for me right now. >> > > >> > > >> > > >> > > I'm not very familiar with the ISel passes. I'm guessing the correct >> > > thing to do here would be to apply the attribute correctly in FastISel >> > > so it works, but I'm kind of unfamiliar with this part of the code and >> > > won't have time to dig into it right now, although I'm willing to do >> > > it if someone points me to the right area of code. >> > > >> > > >> > > >> > > In the meantime, would it be worthwhile to submit in a modification to >> > > disable FastISel for non-darwin targets, or is it likely to be fixed >> > > quickly? >> > > >> > > >> > > >> > > -Gordon >> > > >> > > >> > > _______________________________________________ >> > > LLVM Developers mailing list >> > > LLVMdev at cs.uiuc.edu<mailto:LLVMdev at cs.uiuc.edu> http://llvm.cs.uiuc.edu >> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > > >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu<mailto:LLVMdev at cs.uiuc.edu> http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu<mailto:LLVMdev at cs.uiuc.edu> http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-- With best regards, Anton Korobeynikov Faculty of Mathematics and Mechanics, Saint Petersburg State University _______________________________________________ LLVM Developers mailing list LLVMdev at cs.uiuc.edu<mailto:LLVMdev at cs.uiuc.edu> http://llvm.cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130821/15c56307/attachment.html>