This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Add a mapping from "select register" to "load on condition" (2-addr).
ClosedPublic

Authored by jonpa on Oct 11 2019, 7:57 AM.

Details

Reviewers
uweigand
Summary

The SELR(Mux) instructions can be converted to two-address form as LOCR(Mux) instructions whenever one of the sources are the same reg as dest. By adding this mapping in getTwoOperandOpcode(), we get:

  • Two address hints in getRegAllocationHints() for select register instructions.
  • No effect in foldMemoryOperandImpl() as the select register instructions do not have a memory mapped instructions. This is possibly a potential improvement to be utilized as is currently done for the K (3-address) instructions.
  • no need for special handling in SystemZShortenInst.cpp - shortenSelect() removed.

It seems that there is yet another part of the "high-muxes patch" that never got committed separately, namely the moving of the handling of two-address hints to be done before the GRX32 hints, in getRegAllocationHints(), which this patch also does.

Instruction mix effects on SPEC 2006:

selgrlh        :                 1728                  231    -1497
locgre         :                 1923                 3385    +1462
selgre         :                 1511                  753     -758
locgrlh        :                 1159                 1861     +702
selgrl         :                 1175                  499     -676
locrl          :                 1354                 1996     +642
selgrnle       :                  641                   82     -559
locgrle        :                  356                  905     +549
locgrhe        :                  779                 1320     +541
selrl          :                 1081                  541     -540
locgrl         :                  722                 1059     +337
selrh          :                  536                  209     -327
ahi            :                31094                31403     +309
ahik           :                26654                26351     -303
locrle         :                  502                  749     +247
locre          :                  761                  977     +216
selgrhe        :                  216                    4     -212
selgrh         :                  489                  277     -212
selre          :                  333                  162     -171
...

Number of spill instructions go up just a little in total:

master <> patch
Spill|Reload   :               187260               187339      +79
Copies         :               414305               414303       -2

This seems to be small enough to be considered random. It seems that the number of spilled live ranges in total are very slightly less, while the number of instructions emitted for spill/restore are very slightly more... I could not find any reason to suspect anything in particular with the test cases I tried to examine, and it should hopefully not be bad to hint (softly) a two-address register.

738 files are different.

Diff Detail

Event Timeline

jonpa created this revision.Oct 11 2019, 7:57 AM

What is the effect of this part?

It seems that there is yet another part of the "high-muxes patch" that never got committed separately, namely the moving of the handling of two-address hints to be done before the GRX32 hints, in getRegAllocationHints(), which this patch also does.

In any case, this should better be done as a separate commit.

no need for special handling in SystemZShortenInst.cpp - shortenSelect() removed.

Do those really never match again now? But I agree it would be nice to remove those if possible.

lib/Target/SystemZ/SystemZInstrFormats.td
3230 ↗(On Diff #224590)

I'd prefer to move this into CondBinaryRRF itself, so that it matches CondBinaryRRFPseudo again.

3265 ↗(On Diff #224590)

Likewise for CondBinaryRRFa.

jonpa updated this revision to Diff 224732.Oct 12 2019, 1:44 AM
jonpa marked 3 inline comments as done.

Patch updated (SystemZInstrFormats.td).

What is the effect of this part?

It seems that there is yet another part of the "high-muxes patch" that never got committed separately, namely the moving of the handling of two-address hints to be done before the GRX32 hints, in getRegAllocationHints(), which this patch also does.

In any case, this should better be done as a separate commit.

The effect of applying just the change in getRegAllocationHints() to move the 2-address hints above GRX32 hints is on SPEC 2006 / z15:

22 files are different.

master<> patch
ahik           :                26654                26355     -299
ahi            :                31094                31393     +299
locrhe         :                  659                  470     -189
locrl          :                 1354                 1468     +114
selrl          :                 1081                 1156      +75
l              :                73744                73751       +7
lgr            :               345992               345998       +6
lr             :                26243                26248       +5
lg             :               374181               374177       -4
stg            :               140079               140076       -3
stfh           :                 3288                 3291       +3
a              :                13806                13803       -3
ar             :                19536                19539       +3
iihf           :                 2271                 2274       +3
llihl          :                 2123                 2120       -3
risbhg         :                 3387                 3384       -3
cg             :                10168                10166       -2
cgrjhe         :                 4899                 4901       +2
jhe            :                10882                10880       -2

Spill|Reload   :               187260               187264       +4
Copies         :               414305               414313       +8

It seems that some two-address hints now are passed for instructions that use the same regs as a SELRMux, so there are more AHI:s and less AHIK:s. By chance - since we are not handling the SELRMux instructions here - we also get a few more SELR:s and a few less LOCR:s.

I could pre-commit this, or we could perhaps do benchmarking on the full patch (I see some mixed minor variations in my own runs).

no need for special handling in SystemZShortenInst.cpp - shortenSelect() removed.

Do those really never match again now? But I agree it would be nice to remove those if possible.

I checked it once on 2006 and it was NFC to remove them. shortenSelect() changes opcode and ties the operands if it can - using commuteInstruction() if helpful - and this is exactly what the default handling for a TwoOperandOpcode does as well.

jonpa added inline comments.Oct 12 2019, 1:46 AM
lib/Target/SystemZ/SystemZInstrFormats.td
3230 ↗(On Diff #224590)

ok, makes sense.

jonpa updated this revision to Diff 234338.Dec 17 2019, 10:32 AM

Patch rebased, no manual changes needed...

Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2019, 10:32 AM
uweigand accepted this revision.Dec 20 2019, 7:18 AM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 20 2019, 7:18 AM
jonpa closed this revision.Dec 20 2019, 10:47 AM

Thanks for review!

9fcebad