This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] convergent intrinsics
ClosedPublic

Authored by sameerds on Jul 8 2023, 6:09 AM.

Details

Summary

Introduced the convergent equivalent of the existing G_INTRINSIC opcodes:

  • G_INTRINSIC_CONVERGENT
  • G_INTRINSIC_CONVERGENT_W_SIDE_EFFECTS

Out of the targets that currently have some support for GlobalISel, the patch
assumes that the convergent intrinsics only relevant to SPIRV and AMDGPU.

Diff Detail

Event Timeline

sameerds created this revision.Jul 8 2023, 6:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2023, 6:09 AM
sameerds requested review of this revision.Jul 8 2023, 6:09 AM
Herald added a project: Restricted Project. · View Herald Transcript
sameerds added a subscriber: Restricted Project.
yassingh added inline comments.
llvm/include/llvm/CodeGen/MachineInstr.h
1373–1383 ↗(On Diff #538362)

Can this fit in GenericMachineInstrs.h? Also the name can be modified a bit to reflect it only queries generic instructions.

llvm/lib/CodeGen/MachineVerifier.cpp
1560–1561

Will these error messages suffice for CONVERGENT intrinsics too?

tschuett added inline comments.
llvm/docs/GlobalISel/GenericOpcode.rst
859–860

Could you please out all four of them? It is easier to read for me.

863–864

I would argue that it has to be plural. the x variants.

arsenm added inline comments.Jul 11 2023, 11:58 AM
llvm/include/llvm/CodeGen/MachineInstr.h
1948 ↗(On Diff #538362)

I don't think we should spread using intrinsic operands for anything else? I'd hope the verifier didn't allow this

llvm/lib/CodeGen/MachineVerifier.cpp
1560–1561

Should implement the matching convergent intrinsic declaration checks

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
602–604

This is more permissive than before

arsenm added inline comments.Jul 11 2023, 12:05 PM
llvm/utils/TableGen/GlobalISelEmitter.cpp
869

.startswith("G_INTRINSIC")?

sameerds updated this revision to Diff 539919.Jul 13 2023, 3:15 AM
sameerds marked 8 inline comments as done.
  • rebase
  • addressed review comments
sameerds added inline comments.Jul 13 2023, 3:17 AM
llvm/docs/GlobalISel/GenericOpcode.rst
859–860

Trying a different way to mention the variants. Hope this one makes sense!

llvm/include/llvm/CodeGen/MachineInstr.h
1373–1383 ↗(On Diff #538362)

Nice catch! This also provided a way to resolve other comments about isIntrinsic().

1948 ↗(On Diff #538362)

This comment was tersely trying to say that isIntrinsic() may not capture all the generic intrinsics, such as target-defined G_INTRINSIC_* opcodes. But the latest version now drops introducing isIntrinsic() and instead introduces isa<GIntrinsic>() which is easier to describe.

llvm/lib/CodeGen/MachineVerifier.cpp
1560–1561

Fixed it by printing the actual name of the intrinsic.

llvm/utils/TableGen/GlobalISelEmitter.cpp
869

I tried that, but it fails with other opcodes that start with G_INTRINSIC but are not really intrinsics. That change needs to happen separately, and can include this cleanup.

arsenm added inline comments.Jul 13 2023, 6:26 AM
llvm/include/llvm/CodeGen/MachineInstr.h
1948 ↗(On Diff #538362)

Targets probably shouldn't define their own G_INTRINSIC instructions. They can define their own G_* instructions, but seems unnecessary they would need the abstraction layer to track different intrinsics in them

llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
779

I'd expect this to use another bool parameter along with HasSideEffects.

I'm also thinking ahead to noconvergent call sites

797–798

Ditto

sameerds marked 3 inline comments as done.Jul 16 2023, 8:45 PM
sameerds added inline comments.
llvm/include/llvm/CodeGen/MachineInstr.h
1948 ↗(On Diff #538362)

I am not seeing an actionable comment here. The status before this change was that getIntrinsicID() is used in some places where the opcode is not a generic intrinsic, but the client still expects to find an Intrinsic::ID in the same place. Those places broke when I put an isIntrinsic() assert. This change does keeps the status quo. The only contribution here is an improvement in having a single predicate for generic intrinsic instead of checking four opcodes.

llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
779

If you mean a parameter from the TableGen description, then this is the place where the isConvergent parameter from the def of the intrinsic gets translated to the isConvergent parameter in the def of G_INTRINSIC_CONVERGENT_*. The use of getFnAttributes on the intrinsic declaration is how we access the former, and the opcode selection inside getIntrinsicCode is how we set the latter.

arsenm added inline comments.Jul 17 2023, 4:36 PM
llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
779

I mean buildIntrinsic has a HasSideEffects bool parameter. I expect another partner operand for convergent, rather than looking it up through the declaration here. A different buildIntrinsic overload that queries the underlying declaration is potentially useful, but I don't want to mix the two here

sameerds updated this revision to Diff 541897.Jul 19 2023, 1:56 AM
sameerds marked 2 inline comments as done.
  • rebase
  • fix parameters to buildIntrinsic()
sameerds added inline comments.
llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
779

Fixed this by introducing an explicit version that takes two boolean arguments. The default version determines both parameters (side-effects and convergent) from the intrinsic ID, and then passes that to the explicit version. I believe this is a strong way to lay out what is intended.

sameerds marked an inline comment as done.Jul 26 2023, 10:30 PM
sameerds added inline comments.
llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
779

Bump!

foad added inline comments.Jul 27 2023, 3:33 AM
llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
1111

Comment needs to explain what the non-"Explicit" ones do.

Do you actually need the Explicit suffix? You could just leave them all as different overloads of buildIntrinsic.

llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
778

s/Code/Opcode/

llvm/lib/CodeGen/MachineVerifier.cpp
228–229

I find these names weird, because verifyGIntrinsicWithSideEffects does not just verify G_INTRINSIC_W_SIDE_EFFECTS. It verifies the side-effect-ness of all intrinsics.

Maybe rename to:
verifyGIntrinsicSideEffects
verifyGIntrinsicConvergence
?

Or just re-inline them back into their only caller?

sameerds updated this revision to Diff 545085.Jul 28 2023, 3:13 AM
  • rebased
  • replaced the "Explicit" functions with overloads
  • s/Code/Opcode/
  • renamed verifyIntrinsic* functions
sameerds marked 3 inline comments as done.Jul 28 2023, 3:15 AM

LGTM except for the documentation sentence no longer making sense

llvm/docs/GlobalISel/GenericOpcode.rst
868–880

If you drop the "is no void variant" part, the "Unlike SelectionDAG" part doesn't make sense. Either preserve the first sentence entirely or drop it

llvm/lib/CodeGen/MachineVerifier.cpp
971

In principle we could have local call site attributes override, but it requires more work to really support

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
4817–4820

Really we should add buildIntrinsic that takes operand lists like other buildInstr wrappers

This revision was not accepted when it landed; it landed in state Needs Review.Jul 30 2023, 11:46 PM
This revision was automatically updated to reflect the committed changes.
sameerds added inline comments.Jul 30 2023, 11:46 PM
llvm/docs/GlobalISel/GenericOpcode.rst
868–880

Kept the original.