This is an archive of the discontinued LLVM Phabricator instance.

[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

Repository
rL LLVM

Event Timeline

NikolaPrica created this revision.Apr 3 2018, 2:39 AM
efriedma added inline comments.Apr 10 2018, 11:30 AM
lib/Target/ARM/ARMInstrNEON.td
5065 ↗(On Diff #140747)

An "or" is only a move if the two inputs are the same register; does that really meet the requirements of isMoveReg?

lib/Target/ARM/ARMInstrThumb.td
1158 ↗(On Diff #140747)

For targets older than armv6, "movs" is a lot more common than "mov"; you probably want to handle that too?

lib/Target/ARM/ARMInstrVFP.td
1027 ↗(On Diff #140747)

We usually use VMOVRRD (which moves two 32-bit integers into one 64-bit register) rather than VMOVRS; is there some way you can treat that as a "copy"?

NikolaPrica marked 2 inline comments as done.Apr 11 2018, 2:18 AM
NikolaPrica added inline comments.
lib/Target/ARM/ARMInstrVFP.td
1027 ↗(On Diff #140747)

There is similar instruction for MIPS that I've covered. On MIPS architecture similar instruction is used to transfer function arguments that were previously held in 32-bit register but for purpouse of call are transfered into 64-bit. I think that it should be treated as copy?

[ARM] Consider VORRq as copy instruction when two inputs are the same register

Treat tMOVsr as isMoveReg
Treat VMOVRRD as isMoveReg
craig.topper added inline comments.Apr 11 2018, 10:41 AM
lib/Target/X86/X86InstrAVX512.td
2771 ↗(On Diff #141974)

Why is it disabled for this class and KMOVB above?

3243 ↗(On Diff #141974)

This should be tagged I think.

3323 ↗(On Diff #141974)

Why was this line deleted?

3343 ↗(On Diff #141974)

This should be tagged I think.

lib/Target/X86/X86InstrInfo.td
1616 ↗(On Diff #141974)

What about these?

lib/Target/X86/X86InstrMMX.td
295 ↗(On Diff #141974)

Why isn't MMX_MOVQ64rr_REV tagged?

lib/Target/X86/X86InstrSSE.td
650 ↗(On Diff #141974)

Shouldn't this be move reg?

743 ↗(On Diff #141974)

Why is this only on VMOVAPSrr_REV and not the 3 following instructions?

ARM changes look fine.

lib/Target/ARM/ARMInstrVFP.td
1027 ↗(On Diff #140747)

Not sure how the target-independent code will understand the semantics for a "move" with multiple inputs, but I'll let you figure that out, I guess.

NikolaPrica marked an inline comment as not done.

Thank 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:
-Added isMoveReg for table definiton class avx512_mask_mov_gpr ???
-Remove disabling flags for KMOVB and KMOVD
-Added isMoveReg for table definiton class avx512_load and avx512_store for "def rr" and "def rr_REV"
-Tagged MOV*rr_REV instructions as isMoveReg
-Tagged MMX_MOVQ64rr_REV as isMoveReg
-Added isMoveReg for table definition class sse12_mov_packed for "def rr"
-Tagged VMOVAP[S|D][Y]rr_REV, VMOVUP[S|D][Y]rr_REV, MOVAP[S|D]rr_REV and MOVUP[S|D]rr_REV as isMoveREg ???

NikolaPrica added inline comments.Apr 12 2018, 5:55 AM
lib/Target/ARM/ARMInstrVFP.td
1027 ↗(On Diff #140747)

In following patch new destination register is recognized as one that is marked as <def> and the source register is recognized only if it is marked as <killed>. Do you think that this will be enough?

lib/Target/X86/X86InstrAVX512.td
3323 ↗(On Diff #141974)

Oh, this got here by mistake. I've overlooked it.

craig.topper added inline comments.Apr 12 2018, 10:00 AM
lib/Target/X86/X86InstrAVX512.td
2749 ↗(On Diff #142147)

The widths of the registers don't always match here. Is that a problem? Bits are either truncated or zero extended.

NikolaPrica added inline comments.Apr 12 2018, 10:57 AM
lib/Target/X86/X86InstrAVX512.td
2749 ↗(On Diff #142147)

If bits are truncated then it can be a problem. I will remove this then.

Removed isMoveReg from avx512_move_gpr

efriedma added inline comments.Apr 12 2018, 11:17 AM
lib/Target/ARM/ARMInstrVFP.td
1027 ↗(On Diff #140747)

The issue with vmovrrd in particular is that there are multiple source registers; the current patch in D44016 only looks for one of them.

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.

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.

There is already such hook for some mips and arm instructions like that.

lib/Target/ARM/ARMInstrVFP.td
1027 ↗(On Diff #140747)

Oh. I've looked closer now. That could be a problem. Currently I'm not sure how to handle this in proper general way. It might be best to leave this as FIXME in ARMBaseInfo::isCopyInstr and untagg VMOVRRD since this is sort of a corner case?

Added FIXME in ARMBaseInstrInfo::isCopyInstr
Untagged isMoveReg from VMOVRRD

kparzysz added a comment.EditedApr 13 2018, 6:19 AM

There is already such hook for some mips and arm instructions like that.

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.

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.

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.

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.

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.

@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?
@sdardis What do you think about this?

A while back, with Hal, we talked about having something like:
bool TargetInstrInfo::isEquivalentTo(GenericOpcode, ExpectedOperand)

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.
The nice thing about such a thing is, thanks to GISel, we now have a way to express common structure like G_ADD.

(Credit to Hal for suggesting that.)

@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.

Hi,

The general direction makes sense and I agree you probably don't want to implement the full isEquivalentTo stuff I was talking about.
A general pointer you might have missed. We already track copy like instructions through isRegSequenceLike, isExtractSubRegLike and so on. These are more complicated than just plain copies though (e.g., VMOVRRD should already be supported).

Random thoughts:

  • you may want to teach the peephole optimizer how to use that information to eliminate more copies.
  • why can't you do your analysis before the instructions are lowered (i.e., when all that stuff are still copies).

Cheers,
-Quentin

lib/Target/ARM/ARMBaseInstrInfo.cpp
941 ↗(On Diff #146148)

VMOVRRD should already fall into the "extract sub reg" category. So there shouldn't be anything to do here.

A general pointer you might have missed. We already track copy like instructions through isRegSequenceLike, isExtractSubRegLike and so on. These are more complicated than just plain copies though (e.g., VMOVRRD should already be supported).

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?

  • you may want to teach the peephole optimizer how to use that information to eliminate more copies.

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?

  • why can't you do your analysis before the instructions are lowered (i.e., when all that stuff are still copies).

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.

As you can see I left VMOVRRD ARM instruction as FIXME since it is more complicated copy.

Yes, I saw that :).

I'm not quite sure whether I should consolidate getRegSequenceInputs and getExtractSubregInputs into isCopyInstr or just use them as they are?

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.

Or maybe just rename isCopyInstr into isSimpleCopyInstr?

Up to you.

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?

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).

[...] only at this pass existing DBG_VALUEs are propagated to other blocks.

I see, that's unfortunate but I understand.

Updated just comment in ARMBaseInstrInfo.cpp

If there is no more comments and everything looks fine, can someone approve this?

qcolombet accepted this revision.May 22 2018, 6:24 PM
This revision is now accepted and ready to land.May 22 2018, 6:24 PM

Updating patch to match latest trunk version.

This revision was automatically updated to reflect the committed changes.

In a lot of cases, we would like to treat MoveReg-type instructions and generic COPY instruction in the same way.
E.g. in this code:

// 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
@@ -3114,7 +3114,7 @@ void X86InstrInfo::copyPhysReg(MachineBasicBlock &MBB,
bool X86InstrInfo::isCopyInstr(const MachineInstr &MI,

const MachineOperand *&Src,
const MachineOperand *&Dest) const {
  • if (MI.isMoveReg()) {

+ if (MI.isCopy() || MI.isMoveReg()) {

Dest = &MI.getOperand(0);
Src = &MI.getOperand(1);
return true;

Do you think it makes sense?

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.

Thanks, I submitted this idea for a review here: https://reviews.llvm.org/D49913