Hi Eli, On 07/05/2012 18:15, Eli Friedman wrote:> On Mon, May 7, 2012 at 5:15 AM, Ivan Llopard<ivanllopard at gmail.com> wrote: >> Hi all, >> >> Tuning my TargetAsmPrinter implementation in the back-end side, I >> discovered that the address space number is not passed down while >> emitting global variables with constant initializers. The information is >> dropped at AsmPrinter::EmitGlobalConstant() function call which defaults >> it to zero. > > This sounds like a bug.The feature added to allow targets emit custom data directives based on the address space seems to be broken. However, it doesn't produce any odd behavior because it's not used. The attached patch is quite small and shows where the address space information is getting lost and it will correctly propagate it. It also avoids existent back-ends to break because of an unimplemented hook in MCAsmInfo. Ivan> > -Eli-------------- next part -------------- A non-text attachment was scrubbed... Name: addrspace-asmprinter.patch Type: text/x-patch Size: 2018 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120508/e0712e29/attachment.bin>
Ivan,> The attached patch is quite small and shows where the address space > information is getting lost and it will correctly propagate it. It also > avoids existent back-ends to break because of an unimplemented hook in > MCAsmInfo.Testcase, please, if possible :) -- With best regards, Anton Korobeynikov Faculty of Mathematics and Mechanics, Saint Petersburg State University
On Tue, May 8, 2012 at 4:59 AM, Ivan Llopard <ivanllopard at gmail.com> wrote:> Hi Eli, > > > On 07/05/2012 18:15, Eli Friedman wrote: >> >> On Mon, May 7, 2012 at 5:15 AM, Ivan Llopard<ivanllopard at gmail.com> >> wrote: >>> >>> Hi all, >>> >>> Tuning my TargetAsmPrinter implementation in the back-end side, I >>> discovered that the address space number is not passed down while >>> emitting global variables with constant initializers. The information is >>> dropped at AsmPrinter::EmitGlobalConstant() function call which defaults >>> it to zero. >> >> >> This sounds like a bug. > > > The feature added to allow targets emit custom data directives based on the > address space seems to be broken. However, it doesn't produce any odd > behavior because it's not used. > The attached patch is quite small and shows where the address space > information is getting lost and it will correctly propagate it. It also > avoids existent back-ends to break because of an unimplemented hook in > MCAsmInfo.@@ -1046,7 +1046,8 @@ if (CPE.isMachineConstantPoolEntry()) EmitMachineConstantPoolValue(CPE.Val.MachineCPVal); else - EmitGlobalConstant(CPE.Val.ConstVal); + EmitGlobalConstant(CPE.Val.ConstVal, (isa<PointerType>(Ty)) ? + cast<PointerType>(Ty)->getAddressSpace() : 0); } } } This looks suspicious; how can an entry in the constant pool possibly acquire an address space based on the type of the value? -Eli
Le 08/05/2012 20:44, Eli Friedman a écrit :> On Tue, May 8, 2012 at 4:59 AM, Ivan Llopard<ivanllopard at gmail.com> wrote: >> Hi Eli, >> >> >> On 07/05/2012 18:15, Eli Friedman wrote: >>> On Mon, May 7, 2012 at 5:15 AM, Ivan Llopard<ivanllopard at gmail.com> >>> wrote: >>>> Hi all, >>>> >>>> Tuning my TargetAsmPrinter implementation in the back-end side, I >>>> discovered that the address space number is not passed down while >>>> emitting global variables with constant initializers. The information is >>>> dropped at AsmPrinter::EmitGlobalConstant() function call which defaults >>>> it to zero. >>> >>> This sounds like a bug. >> >> The feature added to allow targets emit custom data directives based on the >> address space seems to be broken. However, it doesn't produce any odd >> behavior because it's not used. >> The attached patch is quite small and shows where the address space >> information is getting lost and it will correctly propagate it. It also >> avoids existent back-ends to break because of an unimplemented hook in >> MCAsmInfo. > @@ -1046,7 +1046,8 @@ > if (CPE.isMachineConstantPoolEntry()) > EmitMachineConstantPoolValue(CPE.Val.MachineCPVal); > else > - EmitGlobalConstant(CPE.Val.ConstVal); > + EmitGlobalConstant(CPE.Val.ConstVal, (isa<PointerType>(Ty)) ? > + cast<PointerType>(Ty)->getAddressSpace() : 0); > } > } > } > > This looks suspicious; how can an entry in the constant pool possibly > acquire an address space based on the type of the value?IIUC, global values are also constants, I added that verification just in case. But I agree that it's may be unrealistic. Ivan> > -Eli
Hi Anton, No back-end uses this information, so I don't know if it's worth to put this upstream. Following the approach that Joerg described very well, a section change and a linker script can take care of it, instead of customizing basic data directives based on the address space. It turns to be sane and good enough for me. Ivan Le 08/05/2012 15:08, Anton Korobeynikov a écrit :> Ivan, > >> The attached patch is quite small and shows where the address space >> information is getting lost and it will correctly propagate it. It also >> avoids existent back-ends to break because of an unimplemented hook in >> MCAsmInfo. > Testcase, please, if possible :) >