Paths
| Differential D62653
[Mips][DSP] Fix physregs incorrectly marked as dead. Needs ReviewPublic Authored by mbrkusanin on May 30 2019, 5:15 AM.
Details Summary Implicit operands that use DSPControl register such as RDDSP and WRDSP are added We also don't need removing of dead flags in LiveVariable analysis pass anymore.
Diff Detail Event TimelineComment Actions The changes can be seen in some functions in test CodeGen/Mips/dsp-r1.ll. Where we previously had: CMPU_EQ_QB killed %4:gpr32, killed %3:gpr32, implicit-def dead $dspccond We now have: CMPU_EQ_QB killed %4:gpr32, killed %3:gpr32, implicit-def $dspccond Comment Actions I would like to avoid having "undead" registers, which are marked dead but are semantically necessary, at any point. That means never calling setIsDead(false); as part of instruction selection, even if the flags are eventually correct after post-isel hooks run. The reason for this is that otherwise it's very confusing for anyone reading the code, or MIR dumps, to understand how it's supposed to work. Why aren't these registers modeled precisely during isel? Comment Actions
The idea was to remove setIsDead(false); which was already in LiveVariables.cpp. This way flags are corrected earlier.
That is the main issue. Because some registers for rddsp or wrdsp are added in post isel hook, the previous check which looks at uses in glue chain does not see it. As for modeling the instruction, I don't see any possible way of adding these registers to .td for these instructions. Is there some other place that you know of or have any suggestion where that could be done? I would prefer that too. Comment Actions I think fundamentally, the problem here is that the registers in question aren't being handled consistently. Looking briefly, at the IR level, the DSPCtrl registers are implicit: it's equivalent to some unknown location in memory. Then, during isel, some instructions acquire explicit references to those registers. Then, after isel, some other instructions acquire explicit references to those registers. This is fundamentally broken; you have to transition from modeling DSPCtrl as unknown memory to modeling it as an explicit set of registers at exactly one point, for all operations which refer to DSPCtrl. Otherwise the dependencies are wrong. This doesn't just affect the dead/undef markings; this could also have implications for scheduling. For example, if we don't model the dependency between wrdsp and pick.qb, we could schedule them in the wrong order. Not sure where the best place to convert from arbitrary "memory" to specific registers is. Probably the simplest is to do it all in post-isel hooks? Comment Actions Problem with handling everything in post-isel hooks is that it is after marking registers as dead. So we would need to mark every instruction that uses any part of DSPControl as having an post-isel hook and there are more than 100 instructions (including MicroMips variants). Comment Actions All changes are now in code for Mips target. Only change in common code is in removing there lines in LiveVariables.cpp that are preceded by a FIXME comment. Also, tests that check other DSPControl registers are added. Comment Actions
It should be possible to do this without too much boilerplate using a TSFlags bit; you can mark all the instructions that need this particular rewrite, and check the bit in processFunctionAfterISel, instead of explicitly listing all the opcodes. The Mips backend uses TSFlags for other instruction properties already; see, for example, IsCTI. Comment Actions Marked all instructions that define part of DSPControl register with TSFlag named defsDSPCtrl. This helps us avoid running unnecessary code on instructions that don't need it. While we could add something similar for RDDSP and WRDSP instructions to make sure all implicit operands are added (which appears that it needs to be done in common CodeGen not Mips) before dead flag are set, it would still not help. This is because other instructions that use DSPControl are not glued to RDDSP and/or WRDSP. So we would still need to check for those instructions some other way which would not be any more efficient then current solution.
mbrkusanin marked an inline comment as done. Comment Actions
mbrkusanin added inline comments.
Comment Actions
If you're going to allow calling llvm.mips.rddsp without some other instruction defining the register, you need to mark the registers live-in to the function. Marking them "undef" means you aren't using the value in the register, which could cause issues later.
Revision Contents
Diff 214088 lib/CodeGen/LiveVariables.cpp
lib/Target/Mips/MCTargetDesc/MipsBaseInfo.h
lib/Target/Mips/MicroMipsDSPInstrInfo.td
lib/Target/Mips/MipsDSPInstrInfo.td
lib/Target/Mips/MipsInstrFormats.td
lib/Target/Mips/MipsInstrInfo.h
lib/Target/Mips/MipsInstrInfo.cpp
lib/Target/Mips/MipsInstrInfo.td
lib/Target/Mips/MipsRegisterInfo.h
lib/Target/Mips/MipsRegisterInfo.cpp
lib/Target/Mips/MipsRegisterInfo.td
lib/Target/Mips/MipsSEISelDAGToDAG.h
lib/Target/Mips/MipsSEISelDAGToDAG.cpp
test/CodeGen/Mips/dsp-implicit-dead.ll
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I'm still not really happy with the setIsDead(false), but I guess it's not a big deal if we aren't actually depending on those register defs for correct scheduling. Could probably use a comment explaining that, though.