This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] introduce MIFlag::NoConvergent
ClosedPublic

Authored by sameerds on Aug 8 2023, 10:51 PM.

Details

Summary

Some opcodes in MIR are defined to be convergent by the target by setting
IsConvergent in the corresponding TD file. For example, in AMDGPU, the opcodes
SI_CALL and G_INTRINSIC* are marked as convergent. But this is too conservative,
since calls to functions that do not execute convergent operations should not be
marked convergent. This information is available in LLVM IR.

The new flag MIFlag::NoConvergent now allows the IR translator to mark an
instruction as not performing any convergent operations. It is relevant only on
occurrences of opcodes that are marked convergent. It may be specified on
occurrences of other opcodes, but that's redundant.

Diff Detail

Event Timeline

sameerds created this revision.Aug 8 2023, 10:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 10:51 PM
sameerds requested review of this revision.Aug 8 2023, 10:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 10:51 PM
sameerds added a subscriber: Restricted Project.Aug 8 2023, 10:52 PM

Adding @xbolva00, @RKSimon and @craig.topper since a lot of thought seems to have been involved when the size of MIFlags was bumped to uint32_t, in D118118. Note that we intend to use this new flag MIFlags::NoConvergent only in GlobalISel.

I have a mostly updated copy of the noconvergent patch, fixing the remaining tests seems permanently stuck as the #3 thing on my todo list. I've also been debating splitting the existing isConvergent MI property into two halves (doesn't really matter for this patch, it's a lot easier to change MI flags than IR ones)

I have a mostly updated copy of the noconvergent patch, fixing the remaining tests seems permanently stuck as the #3 thing on my todo list. I've also been debating splitting the existing isConvergent MI property into two halves (doesn't really matter for this patch, it's a lot easier to change MI flags than IR ones)

It's not clear whether you want me to hold this change, or change it in some way.

yassingh added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/CallLowering.h
151

Missed doc comment here

llvm/test/CodeGen/MIR/AMDGPU/noconvergent.mir
2

Is this test auto updatable?

arsenm accepted this revision.Aug 10 2023, 3:02 AM
arsenm added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/CallLowering.h
151

Fix the doc

llvm/test/CodeGen/MIR/AMDGPU/noconvergent.mir
30

Arguably the verifier should reject this

This revision is now accepted and ready to land.Aug 10 2023, 3:02 AM
sameerds updated this revision to Diff 550643.Aug 16 2023, 12:02 AM
  • rebased
  • machine verifier disallows NoConvergent on opcodes that are not isConvergent
  • new test for the machine verifier
  • added missing comment
sameerds marked 3 inline comments as done.Aug 16 2023, 12:03 AM
sameerds added inline comments.
llvm/test/CodeGen/MIR/AMDGPU/noconvergent.mir
30

Yeah, I had thought of silently dropping noconvergent when it is not relevant. But complaining loudly in the machine verifier is a much better idea. Implemented, with test.

arsenm accepted this revision.Aug 17 2023, 1:32 PM
arsenm added inline comments.
llvm/lib/CodeGen/MachineVerifier.cpp
1821–1822

&&

sameerds updated this revision to Diff 551384.Aug 17 2023, 10:23 PM
sameerds marked an inline comment as done.
  • rebased
  • fixed formatting near assert
  • added clang-format off/on to allow existing code through pre-checks
sameerds updated this revision to Diff 551422.Aug 18 2023, 1:18 AM
  • updated the lit test to match the changed error message
arsenm accepted this revision.Aug 18 2023, 5:56 AM
This revision was landed with ongoing or failed builds.Aug 20 2023, 8:46 AM
This revision was automatically updated to reflect the committed changes.