This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Enable scheduling around FP MODE-setting instructions
ClosedPublic

Authored by foad on Sep 10 2020, 3:42 AM.

Details

Summary

Pre-gfx10 all MODE-setting instructions were S_SETREG_B32 which is
marked as having unmodeled side effects, which makes the machine
scheduler treat it as a barrier. Now that we have proper implicit $mode
operands we can use a no-side-effects S_SETMODE_B32 pseudo instead for
setregs that only touch the FP MODE bits, to give the scheduler more
freedom.

Diff Detail

Event Timeline

foad created this revision.Sep 10 2020, 3:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 3:42 AM
foad requested review of this revision.Sep 10 2020, 3:42 AM
arsenm added inline comments.Sep 10 2020, 7:13 AM
llvm/lib/Target/AMDGPU/SOPInstructions.td
847

I don't like defining pseudos that look like real instruction names. I would prefer to call this something like

S_SETREG_B32_mode or something like that

foad updated this revision to Diff 291000.Sep 10 2020, 9:31 AM

Rename the pseudo-instructions.

arsenm added inline comments.Sep 11 2020, 1:46 PM
llvm/lib/Target/AMDGPU/SOPInstructions.td
869

I'm not sure this will be encoded correctly since it's repeating the same instruction definition and not adding the physical aliases. Can you add tests checking the encoding? I think this needs to use PseudoInstExpansion

foad added inline comments.Sep 14 2020, 8:45 AM
llvm/lib/Target/AMDGPU/SOPInstructions.td
869

I'm not sure this will be encoded correctly since it's repeating the same instruction definition and not adding the physical aliases. Can you add tests checking the encoding? I think this needs to use PseudoInstExpansion

869

I'm not sure this will be encoded correctly since it's repeating the same instruction definition and not adding the physical aliases. Can you add tests checking the encoding?

You mean something like adding -show-mc-encoding to test/CodeGen/AMDGPU/llvm.amdgcn.s.setreg.ll ?

I think this needs to use PseudoInstExpansion

I'm not sure how to do that, because at the place where I want to define S_SETREG_IMM32_B32_mode, I don't know what real instruction it should expand to (S_SETREG_B32_gfx6_gfx7 vs S_SETREG_B32_vi vs S_SETREG_B32_gfx10). Is there an example I could follow?

arsenm added inline comments.Sep 14 2020, 1:54 PM
llvm/lib/Target/AMDGPU/SOPInstructions.td
869

Yes.

Oh right, this mechanism doesn't have a way handle multiple alternative encodings. I thought this wasn't an issue for scalar instructions, which I thought had a stable encoding. You could hack around this in emitInstruction. I do think we need better infrastructure for dealing with this problem though.

foad updated this revision to Diff 291864.Sep 15 2020, 4:20 AM

Add -show-mc-encoding.

foad added a comment.Sep 15 2020, 4:21 AM

The encoding seems OK. I can pre-commit the -show-mc-encoding change to make that clearer if you want.

arsenm accepted this revision.Sep 15 2020, 10:02 AM

LGTM, but it would be slightly nicer to factor the instruction definition into a class to avoid repeating the operand definition and asm string

This revision is now accepted and ready to land.Sep 15 2020, 10:02 AM
foad updated this revision to Diff 292222.Sep 16 2020, 7:54 AM

Factor instruction definitions into a class.

arsenm accepted this revision.Sep 16 2020, 8:01 AM
This revision was landed with ongoing or failed builds.Sep 16 2020, 8:11 AM
This revision was automatically updated to reflect the committed changes.