[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,

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

+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.

Hi Konstantin,

Thanks for working on this!

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.

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