This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] GIntrinsic subclass to represent intrinsics in Generic Machine IR
ClosedPublic

Authored by sameerds on Jul 18 2023, 12:49 AM.

Details

Summary

Some opcodes in generic MIR represent calls to intrinsics, where the intrinsic
ID is the first non-def operand to the instruction. These are now represented as
a subclass of GenericMachineInstr, and the method MachineInstr::getIntrinsicID()
is now moved to this subclass GIntrinsic.

Some target-defined instructions behave like GMIR intrinsics, and have an
Intrinsic::ID operand. But they should not be recognized as generic intrinsics,
and should not use GIntrinsic::getIntrinsicID(). Separated these out by
introducing a new AMDGPU::getIntrinsicID().

Diff Detail

Event Timeline

sameerds created this revision.Jul 18 2023, 12:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 12:49 AM
sameerds requested review of this revision.Jul 18 2023, 12:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 12:49 AM
sameerds added a subscriber: Restricted Project.Jul 18 2023, 12:49 AM
arsenm added inline comments.Jul 18 2023, 5:12 AM
llvm/include/llvm/CodeGen/MachineInstr.h
1933

I think making this an assert would be fine?

sameerds added inline comments.Jul 18 2023, 5:53 AM
llvm/include/llvm/CodeGen/MachineInstr.h
1932–1933

@arsenm, do you see any value in introducing GIntrinsic as a subclass of GenericMachineInstr and moving getIntrinsicID() to it? I tried half of that in D154766, but I don't see any real advantage. I am thinking I'll roll it back in D154766.

1933

This allows to reduce verbosity at the client. For example, rather than doing isIntrinsic() { switch (ID) } you can just do switch (ID). Also instead of doing isIntrinsic() && getIntrinsicID() == XX you can simply do getIntrinsicID() == XX.

arsenm added inline comments.Jul 18 2023, 8:33 AM
llvm/include/llvm/CodeGen/MachineInstr.h
1932–1933

We should probably do it and make more use of GenericMachineInstr

sameerds updated this revision to Diff 541843.Jul 18 2023, 10:55 PM
  • Introduce GIntrinsic subclass of GenericMachineInstr
  • Moved MachineInstr::getIntrinsicID() to GIntrinsic
sameerds retitled this revision from [AMDGPU] Isolate target intrinsics that are not GMIR intrinsics. to [GlobalISel] GIntrinsic subclass to represent intrinsics in Generic Machine IR.Jul 18 2023, 10:58 PM
sameerds edited the summary of this revision. (Show Details)
sameerds marked 2 inline comments as done.
sameerds added inline comments.
llvm/include/llvm/CodeGen/MachineInstr.h
1932–1933

Added the new GIntrinsic class and updated the review description.

sameerds marked an inline comment as done.Jul 24 2023, 10:44 PM

Bump! This is blocking further changes. Added some more reviewers to get more eyeballs.

(Adding my eyeballs to the list of eyeballs looking at this)

llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
362

Very nitpicky, but if we're going down the route of using this class more, could we have some is method?
e.g. instead of

cast<GIntrinsic>(MI).getIntrinsicID() == Intrinsic::amdgcn_foo

have

cast<GIntrinsic>(MI).is(Intrinsic::amdgcn_foo)

Also maybe some method like hasSideEffect to check the opcode?

My point is that it's nice to have this type of class, but if we need to fallback to manual checking of the opcode/intrinsic id for any trivial check, this class is just adding verbosity to the code and doesn't add much value, IMO.

llvm/lib/Target/AMDGPU/AMDGPUInstrInfo.h
35–37

I would make the comment a bit more formal, maybe even use Doxygen and give some examples/context so we can easily then when to use this, and when to use GIntrinsic

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
13808

nit: does the LLVM coding style like auto if the type isn't present in the decl?
I love auto but I thought it was only recommended when we have something like cast/dyn_cast where the type is already spelled out on in the init

llvm/lib/Target/SPIRV/SPIRVUtils.cpp
232–235
arsenm added inline comments.Jul 25 2023, 9:15 AM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
362

final?

364

can this be Intrinsic::ID

sameerds updated this revision to Diff 544220.Jul 25 2023, 11:32 PM
sameerds edited the summary of this revision. (Show Details)
  • rebased
  • added the GIntrinsic::is() method
  • fixed issues with coding style
sameerds marked 6 inline comments as done.Jul 25 2023, 11:36 PM
sameerds added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
362

Added both.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
13808

Good point. I am firmly in the "almost always auto" camp myself. But irrespective of everything else, it's generally a good idea to keep existing code as it is.

arsenm accepted this revision.Jul 26 2023, 10:16 AM
This revision is now accepted and ready to land.Jul 26 2023, 10:16 AM
This revision was landed with ongoing or failed builds.Jul 26 2023, 9:31 PM
This revision was automatically updated to reflect the committed changes.
sameerds marked 2 inline comments as done.
vitalybuka added inline comments.
llvm/include/llvm/CodeGen/MachineInstr.h
1935
sameerds reopened this revision.Jul 26 2023, 10:16 PM
This revision is now accepted and ready to land.Jul 26 2023, 10:16 PM
sameerds updated this revision to Diff 544604.Jul 26 2023, 10:16 PM
  • reverted and fixed buildbot breakage
  • there were dangling uses of MachineInstr::getIntrinsicID() in other targets
  • note to self: don't forget to check Phab for a successful build before submitting!
sameerds updated this revision to Diff 544636.Jul 27 2023, 12:49 AM
  • fixed missing headers