This is an archive of the discontinued LLVM Phabricator instance.

Introduce predicate for a atomic operations in GMIR
ClosedPublic

Authored by yassingh on Sep 20 2022, 2:00 AM.

Diff Detail

Event Timeline

yassingh created this revision.Sep 20 2022, 2:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 2:00 AM
yassingh requested review of this revision.Sep 20 2022, 2:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 2:00 AM
sameerds added inline comments.
llvm/include/llvm/Support/TargetOpcodes.def
395

Why is this marker needed? Should OP_END be sufficient to pair with OP_START?

yassingh retitled this revision from Generic atomic instructions identifier function to Introduce predicate for a atomic operations in GMIR.Sep 20 2022, 2:54 AM
yassingh edited the summary of this revision. (Show Details)
yassingh added reviewers: sameerds, arsenm.
yassingh added inline comments.Sep 20 2022, 2:59 AM
llvm/include/llvm/Support/TargetOpcodes.def
395

Yes it's not, OP_START and OP_END should be sufficient, will update

yassingh updated this revision to Diff 461529.Sep 20 2022, 3:18 AM

Removed extra markers

What's this going to be used for?

llvm/include/llvm/Support/TargetOpcodes.def
395

Why doesn't G_FENCE count?

What's this going to be used for?

Atomic instructions are inherently divergent, uniformity analysis need to identify these instructions to mark them divergent

yassingh added inline comments.Sep 20 2022, 7:45 AM
llvm/include/llvm/Support/TargetOpcodes.def
395

I have a doubt too, could not find details about this instruction. Should this also be included?

I don't know how much use a general "isAtomic" predicate will be. You wouldn't really do anything with the same operand indices for all of them. An isAtomicRMW would be more useful?

arsenm added inline comments.Sep 20 2022, 5:28 PM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
114

Opc >= START?

I don't know how much use a general "isAtomic" predicate will be. You wouldn't really do anything with the same operand indices for all of them. An isAtomicRMW would be more useful?

It will need to be "isAtomicRead" to include AtomicLoad and AtomicRMW. Basically any atomic operation that returns a value. We assume that by its very nature of being atomic, the returned value can be different in each thread. Fence is not relevant for this use-case since it does not access memory or return anything; it merely orders operations within the same thread.

yassingh updated this revision to Diff 462111.Sep 22 2022, 1:48 AM

Updated the predicate to check isAtomicRMW operations, added another one for CMPXCGH

arsenm accepted this revision.Sep 22 2022, 6:14 AM
arsenm added inline comments.
llvm/include/llvm/CodeGen/TargetInstrInfo.h
118–121

I think the benefit of this one is dubious. They don't have the same operand structure

This revision is now accepted and ready to land.Sep 22 2022, 6:14 AM
yassingh added inline comments.Sep 22 2022, 6:51 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
118–121

We need this to check divergence of an instruction in GMIR, it was either this or moving this opcode matching inside isDivergent type function which didn't look right and also wasn't consistent with how Atomic instructions are handled in IR divergence/uniformity analysis.

arsenm added inline comments.Sep 22 2022, 6:52 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
118–121

There's nothing wrong with just checking opcodes where you need to. The IR and MIR aren't structured exactly the same way

yassingh added inline comments.Sep 22 2022, 6:56 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
118–121

There's nothing wrong with just checking opcodes where you need to. The IR and MIR aren't structured exactly the same way

Okay. I'll get rid of the second predicate, maybe when the divergence revision comes up we can revisit this if that doesn't look right

yassingh updated this revision to Diff 462171.Sep 22 2022, 7:00 AM
arsenm accepted this revision.Sep 22 2022, 7:05 AM
This revision was automatically updated to reflect the committed changes.