This property is needed in order to follow values movement between registers.
This patch feature is prerequisite for implementing tracking values locations in LiveDebugValues -> https://reviews.llvm.org/D44016.
Paths
| Differential D45204
[X86][MIPS][ARM] New machine instruction property 'isMoveReg' ClosedPublic Authored by NikolaPrica on Apr 3 2018, 2:39 AM.
Details Summary This property is needed in order to follow values movement between registers. This patch feature is prerequisite for implementing tracking values locations in LiveDebugValues -> https://reviews.llvm.org/D44016.
Diff Detail Event TimelineHerald added subscribers: kristof.beyls, arichardson, javed.absar. · View Herald TranscriptApr 3 2018, 2:39 AM
NikolaPrica added inline comments.
Comment Actions [ARM] Consider VORRq as copy instruction when two inputs are the same register Treat tMOVsr as isMoveReg Treat VMOVRRD as isMoveReg
Comment Actions ARM changes look fine.
NikolaPrica marked an inline comment as not done. Comment ActionsThank you for your reviews and expertise @efriedma and @craig.topper ! @craig.topper I wasnt sure wich instructions could be considered as "move register to register" so I've just tagged some of the instructions from copyPhysReg method. In this new diff I've changed the following:
Comment Actions Wouldn't it be better to have this as a TII hook? There are instructions like "or reg, 0" or "add reg, 0" that are effectively copies, but only with specific operands. Comment Actions
There is already such hook for some mips and arm instructions like that.
Comment Actions
Why can't you use that hook then? Make it available for all targets in TargetInstrInfo. I think that adding such bits to the instruction descriptor is a bad idea in general. Edit: I just saw that you actually added a hook, so I take the comment back. My apologies. Comment Actions
I've firstly implemented like that but I've got comment that it might be better to implement it through table description instead of using multiple switch-case constructions for every register move instruction. Comment Actions
Yeah, I just saw that you have added a hook to TII. The problem with bits in MCInstrDesc is that they apply to an opcode, not an instruction as a whole. If a given instruction has a certain property regardless of what operands it has, then the descriptor solution is good. In cases like "is-copy" (and a few others that are already there), it tells you a little, but not enough to be useful. Specifically, in some cases, there is no way to tell whether an instruction is a copy without looking at its operands. Moreover, in cases like "or reg, reg-always-zero", a target-independent pass will see two registers as inputs without knowing which is the one being copied. Excluding instructions that aren't always copies would reduce the accuracy of your analysis without making it easy to improve it later on. Following the logic of isCompare/isBranch, there could be a flag "could-be-copy" with the corresponding "analyzeCopy" or something like that that would tell the caller exactly what gets copied where. I'm not going to oppose committing of this patch in the current form, I don't want it to bounce between two opposing ideas. At the same time I want to emphasize that common interfaces should have enough generality to be useful outside of the current objectives. Comment Actions @kparzysz There are already instructions that have different bits set for different opcodes. But you have a point. In purpose of creating useful interface for TII the best way may be to use such bit isPossilbe[copy/move] to exclude non-copy-instructions and after exclusion return source and destination registers depending on opcode? Or just do it without MCInstDesc exclusion? Comment Actions A while back, with Hal, we talked about having something like: Like, TII->isEquivalentTo(TargetOpcode::COPY, InputMI, [out]MODef, [out]MOSrc), would return true if InputMI is equivalent to a copy and would populate MODef and MOSrc with the related equivalent input and output. (Credit to Hal for suggesting that.) Comment Actions @qcolombet Yes. It would be nice to have such interface but it would require a lot of additional work ( which would need to address all needed generic opcodes). This patch could be part of such work. I've extended isCopyInstr to return source and destination operands. The most usual case is, if it is move instruction, destination operand is 0 and source operand is 1 -> I didn't want to hoist it to the TII::isCopyInstr so it would cause any problems for future architectures that would support isMoveReg machine property. Also for X86 instructions that have suffix _REV is saw in 'evex-to-vex-compress.mir' that they are used as for example "$ymm0 = VMOVAPDYrr_REV $ymm0" so I've let that destination operand is 0 and source is 1. Comment Actions Hi, The general direction makes sense and I agree you probably don't want to implement the full isEquivalentTo stuff I was talking about. Random thoughts:
Cheers,
Comment Actions
Yes. I've missed that. Thanks for that! As you can see I left VMOVRRD ARM instruction as FIXME since it is more complicated copy. I'm not quite sure whether I should consolidate getRegSequenceInputs and getExtractSubregInputs into isCopyInstr or just use them as they are? Or maybe just rename isCopyInstr into isSimpleCopyInstr?
I could check that. But I suppose that it would be better to upgrade it with separate patch since I've already have one patch on the stack that depends on this functionality?
I've considered such approach but this information is needed to perform data flow analysis inside of each basic block in order to conclude in which registers is a variable kept. Such analysis could be performed only during or after the LiveDebugValues which is one of the latest passes. In this pass we propagate certain DBG_VALUE pseudo instructions (whose range in which it is valid is its basic block) in successors blocks that should also contain proper DBG_VALUE instruction. It is the place where we should know which register from each basic block might hold which variable in order to propagate valid variable DBG_VALUE. All DBG_VALUE analysis are performed on basic block level but only at this pass existing DBG_VALUEs are propagated to other blocks. Comment Actions
Yes, I saw that :).
I would just leave them as they are. I was just pointing them out in case you wanted to see how we've done that so far.
Up to you.
Definitely a different patch. I wanted to give other pointer on the kind of usages we may want to do with this callback. why can't you do your analysis before the instructions are lowered (i.e., when all that stuff are still copies).
I see, that's unfortunate but I understand. Comment Actions Updated just comment in ARMBaseInstrInfo.cpp If there is no more comments and everything looks fine, can someone approve this? This revision is now accepted and ready to land.May 22 2018, 6:24 PM Closed by commit rL333093: [X86][MIPS][ARM] New machine instruction property 'isMoveReg' (authored by petarj). · Explain WhyMay 23 2018, 8:32 AM This revision was automatically updated to reflect the committed changes. Comment Actions In a lot of cases, we would like to treat MoveReg-type instructions and generic COPY instruction in the same way. // Analyze copies (which don't overlap themselves). if (MI->isCopy() && !TRI->regsOverlap(MI->getOperand(0).getReg(), MI->getOperand(1).getReg())) { unsigned Def = MI->getOperand(0).getReg(); unsigned Src = MI->getOperand(1).getReg(); >const MachineOperand *DefOp = nullptr; const MachineOperand *SrcOp = nullptr; // Analyze copies (which don't overlap themselves). if (TII->isCopyInstr(*MI, SrcOp, DefOp) && !TRI->regsOverlap(DefOp->getReg(), SrcOp->getReg())) { unsigned DefReg = DefOp->getReg(); unsigned SrcReg = SrcOp->getReg(); But in order for this to work, we need to modify isCopyInstr to include isCopy check: +++ b/lib/Target/X86/X86InstrInfo.cpp const MachineOperand *&Src, const MachineOperand *&Dest) const {
+ if (MI.isCopy() || MI.isMoveReg()) { Dest = &MI.getOperand(0); Src = &MI.getOperand(1); return true; Do you think it makes sense? Comment Actions Yes. I think that it make sense. IsCopyInstr() currently supports check target specific instructions, but with that extension it could be used to check more general instructions. The only thing that this extension needs to worry about is proper setting of source and destination operands, but I suppose that all of this instructions that are considered as IsCopy() have source as operand 1 and destination as operand 0.
Revision Contents
Diff 142147 include/llvm/CodeGen/MachineInstr.h
include/llvm/CodeGen/TargetInstrInfo.h
include/llvm/MC/MCInstrDesc.h
include/llvm/Target/Target.td
lib/Target/ARM/ARMBaseInstrInfo.h
lib/Target/ARM/ARMBaseInstrInfo.cpp
lib/Target/ARM/ARMInstrInfo.td
lib/Target/ARM/ARMInstrThumb.td
lib/Target/ARM/ARMInstrVFP.td
lib/Target/Mips/MicroMipsDSPInstrInfo.td
lib/Target/Mips/MicroMipsInstrFPU.td
lib/Target/Mips/MicroMipsInstrInfo.td
lib/Target/Mips/Mips16InstrInfo.td
lib/Target/Mips/Mips64InstrInfo.td
lib/Target/Mips/MipsDSPInstrInfo.td
lib/Target/Mips/MipsInstrFPU.td
lib/Target/Mips/MipsInstrInfo.td
lib/Target/Mips/MipsMSAInstrInfo.td
lib/Target/Mips/MipsSEInstrInfo.h
lib/Target/Mips/MipsSEInstrInfo.cpp
lib/Target/X86/X86InstrAVX512.td
lib/Target/X86/X86InstrInfo.td
lib/Target/X86/X86InstrMMX.td
lib/Target/X86/X86InstrSSE.td
utils/TableGen/CodeGenInstruction.h
utils/TableGen/CodeGenInstruction.cpp
utils/TableGen/InstrInfoEmitter.cpp
|
VMOVRRD should already fall into the "extract sub reg" category. So there shouldn't be anything to do here.