This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Merge SystemZExpandPseudo pass into SystemZPostRewrite.
ClosedPublic

Authored by jonpa on Sep 11 2019, 1:44 AM.

Details

Reviewers
uweigand
Summary

SystemZExpandPseudo:s only job was to expand LOCRMux instructions into jump sequences. This needs to be done if expandLOCRPseudo() or expandSELRPseudo() fails to find a legal opcode (all registers "high" or "low"). This task has now been moved to SystemZPostRewrite while removing the SystemZExpandPseudo pass.

It is in fact preferred to expand these pseudos directly after register allocation in SystemZPostRewrite since the hinted register combinations are then not subject to later optimizations. A few more handled cases on SPEC 2006 can be seen with this: "Number of LOCRMux jump-sequences": z14: 5 -> 0, arch13: 5 -> 4.

I thought it was a bit simpler and cleaner to keep the expandLOCRPseudo() and expandSELRPseudo() in SystemZInstrInfo. Is it ok to use "public:" and "private:" like this, or should they be moved instead?

If the number of missed cases is a problem (4 on arch13), a next step to try would be the allowHintRecoloring() method as used in D58923, but perhaps that is not quite motivated given the near 0 number of cases left.

Diff Detail

Event Timeline

jonpa created this revision.Sep 11 2019, 1:44 AM

I thought it was a bit simpler and cleaner to keep the expandLOCRPseudo() and expandSELRPseudo() in SystemZInstrInfo. Is it ok to use "public:" and "private:" like this, or should they be moved instead?

I'd prefer to move everything then.

lib/Target/SystemZ/SystemZPostRewrite.cpp
51

What does this change have to do with the rest of the patch?

jonpa updated this revision to Diff 219895.Sep 12 2019, 5:40 AM
jonpa marked 2 inline comments as done.

I moved the functions and renamed them trySelectLOCRMux() and trySelectSELRMux() to avoid confusion (they do not "expand" the LOCRMux). Is there a point in passing the opcodes as arguments to them?

This in turn made some other methods of SystemZInstrInfo required to be "public" in order to use them from SytemZPostRewrite. I made isHighReg() a static method of SystemZRegisterInfo instead, though.

Removed this from expandPostRAPseudo(), since it seemed a bit redundant:

case SystemZ::LOCRMux:
 case SystemZ::SELRMux:
   llvm_unreachable("Should have been handled in SystemZPostRewrite!");
   return true;
jonpa added a comment.Sep 12 2019, 5:44 AM

Wait - is it safe to have SystemZRegisterInfo::isHighReg() be a static function? I remember that you said that static functions were bad in multi-threaded programs. Is the "Reg" argument safe if two threads call this method simultaneously?

lib/Target/SystemZ/SystemZPostRewrite.cpp
51

I figured that since expandLOCRMux() may create new basic blocks, we can't call setPreservesAll(), unless we would be willing to update any analysis passes as needed. I haven't looked into that in detail, but I imagine things like the Dominator Tree and LoopInfo should be recomputed / updated as needed in case expandLOCRMux() is called.

I agree with making isHighReg a static function in SystemZRegisterInfo. I don't see why there would be any issue with a static member function, as long as it has no persistent state. The only issue with multi-threaded programs would be persistant state, e.g. a static member variable or a static variable within the member function. Arguments and (nonstatic) local variables are fine.

I'm not sure we need to expose emitGRX32Move though. In SystemZPostRewrite, can't we just use a COPY to copy values, like we already do elsewhere in this file?

Also, now that selection to existing ISA instruction and expansion to an if-then-else block happen at the same place, I think we can therefore simplify the logic a bit. For example, make expandLOCRMux into a helper routine that creates the if-then-else expansion, and rename trySelectLOCRMux and trySelectSELRMux to selectLOCRMux and selectSELRMux, and have then call the helper if they cannot select to an existing ISA routine directly. This will also avoid first replacing a SELRMux with a LOCRMux just to have the latter being deleted immediately afterwards. (As an additional potential simplication, if the if-then-else expansion helper were to directly support three-address moves, then some of logic in selectSELRMux could be simplified even further.)

lib/Target/SystemZ/SystemZPostRewrite.cpp
51

Ah, this makes sense. Thanks!

jonpa updated this revision to Diff 220063.Sep 13 2019, 2:46 AM

The only issue with multi-threaded programs would be persistant state...

OK, thanks for the explanation.

I'm not sure we need to expose emitGRX32Move though. In SystemZPostRewrite, can't we just use a COPY to copy values, like we already do elsewhere in this file?

Ah, sorry - of course... I also changed the call TII->copyPhysReg() to build a COPY (which was NFC on SPEC 2006).

(As an additional potential simplication, if the if-then-else expansion helper were to directly support three-address moves, then some of logic in selectSELRMux could be simplified even further.)

Since the copying into dest in selectSELRMux() serves both the purpose of increasing the number of selr/selfhr instructions as well as to prepare for expandCondMove(), it seems like the best place to have it to me. I first thought that the commutation should be moved to only be done if needed before calling expandCondMove(). However, I then one case appeared on SPEC 2006 where the commutation had made two SELRs look the same and therefore the CFG optimizer could transform the function to just have one SELR. So I am thinking that it probably is slightly preferred to "canonicalize" the SELRs and increase the chances of them becoming identical. This means I am not sure how this function could actually be simplified...

Since the copying into dest in selectSELRMux() serves both the purpose of increasing the number of selr/selfhr instructions as well as to prepare for expandCondMove(), it seems like the best place to have it to me. I first thought that the commutation should be moved to only be done if needed before calling expandCondMove(). However, I then one case appeared on SPEC 2006 where the commutation had made two SELRs look the same and therefore the CFG optimizer could transform the function to just have one SELR. So I am thinking that it probably is slightly preferred to "canonicalize" the SELRs and increase the chances of them becoming identical. This means I am not sure how this function could actually be simplified...

Ah, I see. This makes sense. The patch looks good to me then we with a just a couple of minor cleanups.

lib/Target/SystemZ/SystemZPostRewrite.cpp
79

Comment should be updated to specify that otherwise, a branch sequence is created.

100

Likewise. Also, MixedOpcode is no longer used and should be removed.

231

I guess this now just be "return true" and we no longer need the Modified variable.

lib/Target/SystemZ/SystemZRegisterInfo.cpp
76

Maybe this small enough so it would sense to move the definition to the header file, so it can be marked inline again.

jonpa updated this revision to Diff 220078.Sep 13 2019, 5:30 AM
jonpa marked 5 inline comments as done.

Updated per review.

lib/Target/SystemZ/SystemZRegisterInfo.cpp
76

I put it next to the other inline:d query methods in SystemZRegisterInfo.h. This makes the total name more manageable as well for the callers, right?

uweigand accepted this revision.Sep 13 2019, 5:59 PM

This LGTM now, thanks!

This revision is now accepted and ready to land.Sep 13 2019, 5:59 PM
jonpa closed this revision.Sep 16 2019, 12:30 AM

r371959

lib/Target/SystemZ/SystemZ.h