It may be moot because Reed is currently rewriting the patch to avoid using EmitInlineAsm and EmitRawText but I wanted to question something here. I'm thinking that hasRawTextSupport() shouldn't be the condition used inside EmitInlineAsm. I think it would be more correct to have a useRawTextSupport() predicate that can return hasRawTextSupport() for (sub)targets that haven't implemented their MC layer, and false for those that have. What do you think?> -----Original Message----- > From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] > On Behalf Of Rafael Espíndola > Sent: 29 January 2014 20:14 > To: Reed Kotler > Cc: LLVMdev at cs.uiuc.edu > Subject: Re: [LLVMdev] making emitInlineAsm protected > > On 28 January 2014 19:56, reed kotler <rkotler at mips.com> wrote: > > I would like to make the following member of AsmPrinter be protected > > > > > > void EmitInlineAsm(StringRef Str, const MDNode *LocMDNode = 0, > > InlineAsm::AsmDialect AsmDialect > > InlineAsm::AD_ATT) const; > > > > I have some stubs that I want to emit in MipsAsmParser . > > You mean Printer? There is no such a thing as inline asm in a .s file. > > > Are there any objections to doing this? > > Probably. EmitInilneAsm is the only case I know that has a reasonable use of > hasRawTextSupport in order for it to work on targets without an asm parser. > Exposing it exposes a way for targets avoid the MC layer. > > Cheers, > Rafael > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Mostly raw text support should only be used for things like comments that don't affect the overall output in the object file either way. I know that Rafael and I want to remove any other use. -eric On Fri, Jan 31, 2014 at 2:26 AM, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:> It may be moot because Reed is currently rewriting the patch to avoid using EmitInlineAsm and EmitRawText but I wanted to question something here. > > I'm thinking that hasRawTextSupport() shouldn't be the condition used inside EmitInlineAsm. I think it would be more correct to have a useRawTextSupport() predicate that can return hasRawTextSupport() for (sub)targets that haven't implemented their MC layer, and false for those that have. What do you think? > >> -----Original Message----- >> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] >> On Behalf Of Rafael Espíndola >> Sent: 29 January 2014 20:14 >> To: Reed Kotler >> Cc: LLVMdev at cs.uiuc.edu >> Subject: Re: [LLVMdev] making emitInlineAsm protected >> >> On 28 January 2014 19:56, reed kotler <rkotler at mips.com> wrote: >> > I would like to make the following member of AsmPrinter be protected >> > >> > >> > void EmitInlineAsm(StringRef Str, const MDNode *LocMDNode = 0, >> > InlineAsm::AsmDialect AsmDialect >> > InlineAsm::AD_ATT) const; >> > >> > I have some stubs that I want to emit in MipsAsmParser . >> >> You mean Printer? There is no such a thing as inline asm in a .s file. >> >> > Are there any objections to doing this? >> >> Probably. EmitInilneAsm is the only case I know that has a reasonable use of >> hasRawTextSupport in order for it to work on targets without an asm parser. >> Exposing it exposes a way for targets avoid the MC layer. >> >> Cheers, >> Rafael >> _______________________________________________ >> 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
On 31 January 2014 05:26, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:> It may be moot because Reed is currently rewriting the patch to avoid using EmitInlineAsm and EmitRawText but I wanted to question something here. > > I'm thinking that hasRawTextSupport() shouldn't be the condition used inside EmitInlineAsm. I think it would be more correct to have a useRawTextSupport() predicate that can return hasRawTextSupport() for (sub)targets that haven't implemented their MC layer, and false for those that have. What do you think?I generally agree. I would probably make it an independent "mature MC support flag". I think that the current uses of hasRawTextSupport can be classified into * Comment printing. Should be moved APIs that only handle comment and have a nop implementation on object streamers. * Bits that have note been moved to MC yet. These need to be implemented. * Some uses in debug printing I have not investigated yet :-) * The one in EmitInlineAsm. The one in EmitInlineAsm will probably be the last to go. The idea behind it I think was that a target could implement object printing but have inline asm still work with -S while the parser is finished. I don't find it too convincing. The most likely reason for implementing object emission for a target is to get "clang -c" (or llc -filetype=obj) working, and that has to handle any inline asm it hits. With an implementation like the one you describe, the development of object emission for a given target would pass these stages. * stage 0: llc -filetype=obj errors. No support has been implemented yet. * stage 1: llc -filetype=obj tries to emit the object, but is know to fail on some constructs (like inline assembly). * stage 2: the streamer is declared mature. It is considered a bug for it to fail. Clang would use this very same flag for deciding if it would use the integrated assembler or not. The big advantage would be that there would be no hasRawTextSupport in the entire code base. For a triple in stage 1, filetype=obj would always fail on inline assembly. For a triple in stage 2 even filetype=asm would parse it. Cheers, Rafael
I believe my suggestion reduces this particular use and moves most targets
further towards the MC layer.
At the moment, EmitInlineAsm is can be written in pseudo code as:
if raw text support is available:
use raw text support
elif the MC layer is available:
use the MC layer
else
raise error
My suggestion swaps those first two conditions so that the pseudo code becomes:
if the MC layer is available and functional:
use the MC layer
elif raw text support is available:
use raw text support
else:
raise error
My main query is about whether swapping the conditions makes sense regardless of
the decision on the original request. That said, swapping the conditions may
reduce your concerns about exposing EmitInlineAsm and using it for the stubs
since the MC path becomes the primary path through the function.
________________________________________
From: Eric Christopher [echristo at gmail.com]
Sent: 31 January 2014 17:19
To: Daniel Sanders
Cc: Rafael Espíndola; Reed Kotler; LLVMdev at cs.uiuc.edu
Subject: Re: [LLVMdev] making emitInlineAsm protected
Mostly raw text support should only be used for things like comments
that don't affect the overall output in the object file either way. I
know that Rafael and I want to remove any other use.
-eric
On Fri, Jan 31, 2014 at 2:26 AM, Daniel Sanders
<Daniel.Sanders at imgtec.com> wrote:> It may be moot because Reed is currently rewriting the patch to avoid using
EmitInlineAsm and EmitRawText but I wanted to question something here.
>
> I'm thinking that hasRawTextSupport() shouldn't be the condition
used inside EmitInlineAsm. I think it would be more correct to have a
useRawTextSupport() predicate that can return hasRawTextSupport() for
(sub)targets that haven't implemented their MC layer, and false for those
that have. What do you think?
>
>> -----Original Message-----
>> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu]
>> On Behalf Of Rafael Espíndola
>> Sent: 29 January 2014 20:14
>> To: Reed Kotler
>> Cc: LLVMdev at cs.uiuc.edu
>> Subject: Re: [LLVMdev] making emitInlineAsm protected
>>
>> On 28 January 2014 19:56, reed kotler <rkotler at mips.com>
wrote:
>> > I would like to make the following member of AsmPrinter be
protected
>> >
>> >
>> > void EmitInlineAsm(StringRef Str, const MDNode *LocMDNode = 0,
>> > InlineAsm::AsmDialect AsmDialect >>
> InlineAsm::AD_ATT) const;
>> >
>> > I have some stubs that I want to emit in MipsAsmParser .
>>
>> You mean Printer? There is no such a thing as inline asm in a .s file.
>>
>> > Are there any objections to doing this?
>>
>> Probably. EmitInilneAsm is the only case I know that has a reasonable
use of
>> hasRawTextSupport in order for it to work on targets without an asm
parser.
>> Exposing it exposes a way for targets avoid the MC layer.
>>
>> Cheers,
>> Rafael
>> _______________________________________________
>> 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
Ok then. I'll put together a patch early next week to add a hasMatureMCSupport() and use it in EmitInlineAsm(). I don't know which targets should return true for hasMatureMCSupport() at the moment though. I guess X86 and ARM would but are there any others? ________________________________________ From: Rafael Espíndola [rafael.espindola at gmail.com] Sent: 31 January 2014 18:53 To: Daniel Sanders Cc: Reed Kotler; LLVMdev at cs.uiuc.edu Subject: Re: [LLVMdev] making emitInlineAsm protected On 31 January 2014 05:26, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:> It may be moot because Reed is currently rewriting the patch to avoid using EmitInlineAsm and EmitRawText but I wanted to question something here. > > I'm thinking that hasRawTextSupport() shouldn't be the condition used inside EmitInlineAsm. I think it would be more correct to have a useRawTextSupport() predicate that can return hasRawTextSupport() for (sub)targets that haven't implemented their MC layer, and false for those that have. What do you think?I generally agree. I would probably make it an independent "mature MC support flag". I think that the current uses of hasRawTextSupport can be classified into * Comment printing. Should be moved APIs that only handle comment and have a nop implementation on object streamers. * Bits that have note been moved to MC yet. These need to be implemented. * Some uses in debug printing I have not investigated yet :-) * The one in EmitInlineAsm. The one in EmitInlineAsm will probably be the last to go. The idea behind it I think was that a target could implement object printing but have inline asm still work with -S while the parser is finished. I don't find it too convincing. The most likely reason for implementing object emission for a target is to get "clang -c" (or llc -filetype=obj) working, and that has to handle any inline asm it hits. With an implementation like the one you describe, the development of object emission for a given target would pass these stages. * stage 0: llc -filetype=obj errors. No support has been implemented yet. * stage 1: llc -filetype=obj tries to emit the object, but is know to fail on some constructs (like inline assembly). * stage 2: the streamer is declared mature. It is considered a bug for it to fail. Clang would use this very same flag for deciding if it would use the integrated assembler or not. The big advantage would be that there would be no hasRawTextSupport in the entire code base. For a triple in stage 1, filetype=obj would always fail on inline assembly. For a triple in stage 2 even filetype=asm would parse it. Cheers, Rafael