llvm dev - Apr 2020 - [GlobalISel] Extended inline assembler support

Hi!

So far, GlobalISel only supports very basic inline assembler constructs 
(no input/output operands, only simple memory clobbers).

In [0], I'm adding support for generic register, immediate, memory and 
clobber constraints.
The code is more or less a direct port from the handling in 
SelectionDAGBuilder.

Before moving on with target specific constraints, I'd like to discuss 
the general strategy for target hooks in GlobalISel.
While most of these hooks on the SelectionDAG side live in the 
TargetLowering class, there is no corresponding GlobalISel subclass of 
the TargetLoweringBase class yet.
Instead, the (one) existing GlobalISel hook currently lives in 
TargetLoweringBase.

For lowering the target specific constraints, we'll have to add a new 
API that creates (G)MIR during IR translation.
Personally I would prefer to have an InlineAsmLowering class, similar to 
the CallLowering class, which implements the generic algorithm and from 
which targets can inherit.
Most of the implementation in [0] could then be moved to that class.

However, this would add yet another class that needs to be wired up in 
the subtarget class. Is this something we would like to avoid?
Is there a better place where target specific GlobalISel hooks should be 
added?

Thanks for your feedback!

Best regards,

Konstantin

[0] https://reviews.llvm.org/D77535

Hi Konstantin,

> On Apr 6, 2020, at 3:11 AM, Konstantin Schwarz via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi!
> 
> So far, GlobalISel only supports very basic inline assembler constructs (no
input/output operands, only simple memory clobbers).
> 
> In [0], I'm adding support for generic register, immediate, memory and
clobber constraints.
> The code is more or less a direct port from the handling in
SelectionDAGBuilder.
Awesome, thanks!
> 
> Before moving on with target specific constraints, I'd like to discuss
the general strategy for target hooks in GlobalISel.
> While most of these hooks on the SelectionDAG side live in the
TargetLowering class, there is no corresponding GlobalISel subclass of the
TargetLoweringBase class yet.
> Instead, the (one) existing GlobalISel hook currently lives in
TargetLoweringBase.
> 
> For lowering the target specific constraints, we'll have to add a new
API that creates (G)MIR during IR translation.
> Personally I would prefer to have an InlineAsmLowering class, similar to
the CallLowering class, which implements the generic algorithm and from which
targets can inherit.
> Most of the implementation in [0] could then be moved to that class.
> 
> However, this would add yet another class that needs to be wired up in the
subtarget class. Is this something we would like to avoid?
> Is there a better place where target specific GlobalISel hooks should be
added?
I don’t think it’s a problem to have another lowering base class for GlobalISel.
IMO it’s not much overhead to inherit and override an existing class versus
having one monolithic lowering class. In terms of organization, there’s nothing
stopping targets from having all of their XYZLowering extensions in a single
file, much like TargetLowering today. If there’s some code re-use that’s
possible between these various lowering classes then that’s a different story.

Thanks,
Amara
> 
> Thanks for your feedback!
> 
> Best regards,
> 
> Konstantin
> 
> [0] https://reviews.llvm.org/D77535
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Mon, Apr 6, 2020 at 3:12 AM Konstantin Schwarz via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> For lowering the target specific constraints, we'll have to add a new
> API that creates (G)MIR during IR translation.
> Personally I would prefer to have an InlineAsmLowering class, similar to
> the CallLowering class, which implements the generic algorithm and from
> which targets can inherit.
> Most of the implementation in [0] could then be moved to that class.
>
> However, this would add yet another class that needs to be wired up in
> the subtarget class. Is this something we would like to avoid?
> Is there a better place where target specific GlobalISel hooks should be
> added?
>
+10 for a new inline asm lowering class that targets extend. The
FooISelLowering.cpp files are super monolithic, and X86ISelLowering.cpp can
take more than a minute to compile. The *CallLowering classes were a much
better design, and I envy them as someone who often works with X86
SDISel call lowering.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200407/f8319050/attachment.html>

Hi Konstantin,

Thanks for working on this!
> On 6 Apr 2020, at 03:11, Konstantin Schwarz via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi!
> 
> So far, GlobalISel only supports very basic inline assembler constructs (no
input/output operands, only simple memory clobbers).
> 
> In [0], I'm adding support for generic register, immediate, memory and
clobber constraints.
> The code is more or less a direct port from the handling in
SelectionDAGBuilder.
> 
> Before moving on with target specific constraints, I'd like to discuss
the general strategy for target hooks in GlobalISel.
> While most of these hooks on the SelectionDAG side live in the
TargetLowering class, there is no corresponding GlobalISel subclass of the
TargetLoweringBase class yet.
> Instead, the (one) existing GlobalISel hook currently lives in
TargetLoweringBase.
> 
> For lowering the target specific constraints, we'll have to add a new
API that creates (G)MIR during IR translation.
> Personally I would prefer to have an InlineAsmLowering class, similar to
the CallLowering class, which implements the generic algorithm and from which
targets can inherit.
> Most of the implementation in [0] could then be moved to that class.
+1. The SelectionDAG implementation is scattered over a few places and each time
I've gone to add something I've missed a bit somewhere. Having it all in
a lowering class for inline assembly would be great. It also means there's a
clear starting point for targets that are adding support for it.
> However, this would add yet another class that needs to be wired up in the
subtarget class. Is this something we would like to avoid?
I don't see a problem with that. If we allow nullptr then it becomes a nice
optional component that targets can add when they need to.

FWIW, there might be an argument for putting it in the LLVMTargetMachine (fewer
instances) and having the users provide the subtarget when they use it but
I'd lean towards putting it in the subtarget for consistency with the rest
of GlobalISel.
> Is there a better place where target specific GlobalISel hooks should be
added?
> 
> Thanks for your feedback!
> 
> Best regards,
> 
> Konstantin
> 
> [0] https://reviews.llvm.org/D77535
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Hi all,

many thanks for all the feedback!

I've created a new revision at https://reviews.llvm.org/D78316 that 
introduces the new InlineAsmLowering class.
On top of that, https://reviews.llvm.org/D78318 and 
https://reviews.llvm.org/D78319 add support for output and input operand 
constraints.

Best regards,

Konstantin

On 08.04.2020 03:31, Daniel Sanders wrote:
> Hi Konstantin,
>
> Thanks for working on this!
>
>> On 6 Apr 2020, at 03:11, Konstantin Schwarz via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
>>
>> Hi!
>>
>> So far, GlobalISel only supports very basic inline assembler constructs
(no input/output operands, only simple memory clobbers).
>>
>> In [0], I'm adding support for generic register, immediate, memory
and clobber constraints.
>> The code is more or less a direct port from the handling in
SelectionDAGBuilder.
>>
>> Before moving on with target specific constraints, I'd like to
discuss the general strategy for target hooks in GlobalISel.
>> While most of these hooks on the SelectionDAG side live in the
TargetLowering class, there is no corresponding GlobalISel subclass of the
TargetLoweringBase class yet.
>> Instead, the (one) existing GlobalISel hook currently lives in
TargetLoweringBase.
>>
>> For lowering the target specific constraints, we'll have to add a
new API that creates (G)MIR during IR translation.
>> Personally I would prefer to have an InlineAsmLowering class, similar
to the CallLowering class, which implements the generic algorithm and from which
targets can inherit.
>> Most of the implementation in [0] could then be moved to that class.
> +1. The SelectionDAG implementation is scattered over a few places and each
time I've gone to add something I've missed a bit somewhere. Having it
all in a lowering class for inline assembly would be great. It also means
there's a clear starting point for targets that are adding support for it.
>
>> However, this would add yet another class that needs to be wired up in
the subtarget class. Is this something we would like to avoid?
> I don't see a problem with that. If we allow nullptr then it becomes a
nice optional component that targets can add when they need to.
>
> FWIW, there might be an argument for putting it in the LLVMTargetMachine
(fewer instances) and having the users provide the subtarget when they use it
but I'd lean towards putting it in the subtarget for consistency with the
rest of GlobalISel.
>
>> Is there a better place where target specific GlobalISel hooks should
be added?
>>
>> Thanks for your feedback!
>>
>> Best regards,
>>
>> Konstantin
>>
>> [0] https://reviews.llvm.org/D77535
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>