On Tuesday 08 December 2009 13:23, Chris Lattner wrote:> Your diff isn't clean and won't apply to mainline, you have some > previously committed changes, like the extraneous #include of > MachineMemOperand.h.Yes, I know. I was simply asking for a review. I can regenerate it if you wish.> More significantly, as I mentioned before, I don't think this is a > great way to go. For MachineOperands on X86, you either have a > register operand (which is obvious whether it is vector or not) or you > have a collection of addressing mode stuff, which is decomposed and > "not an operand".I don't see how a register MachineOperand is obviously a vector unless there's type information somewhere that I'm missing. That decomposed addressing stuff is interesting, because if one of your addressing compoenent is a vector, you have a gather-scatter situation.> What is the expected use case for "vector" operands that are not > registers? What do you plan to use this information for?Well, as I explained earlier, I wanted to add type information to MachinbeMemOperands so I could comment spills as either Vector or Scalar. That's less important now, so that's why I decided to drop that for the time being. I still think type information in the MachineMemOperand is a good idea because it preserves useful information longer. But I'll come back to that if/when I have a stronger need. As for this patch, which marks instructions and operands as vector, I don't have an immediate use other than it originally drove the vector/scalar spill marking before Evan (quite rightly) asked for a more general approach to deciding if an instruction is a vector operation. At that point I decided that the MachineMemOperand needed to change to mark spills. In the future, I could imagine this being useful for doing peeps and other things where we might like to know the cost of an instruction, analyze the % of a function vectorized, etc. For now, I just don't want to lose this work. Even just applying this and immediately reverting it is better than nothing because it will at least be preserved in the repository history. -Dave
On Dec 8, 2009, at 12:08 PM, David Greene wrote:> On Tuesday 08 December 2009 13:23, Chris Lattner wrote: > >> Your diff isn't clean and won't apply to mainline, you have some >> previously committed changes, like the extraneous #include of >> MachineMemOperand.h. > > Yes, I know. I was simply asking for a review. I can regenerate it > if you wish. > >> More significantly, as I mentioned before, I don't think this is a >> great way to go. For MachineOperands on X86, you either have a >> register operand (which is obvious whether it is vector or not) or you >> have a collection of addressing mode stuff, which is decomposed and >> "not an operand". > > I don't see how a register MachineOperand is obviously a vector unless > there's type information somewhere that I'm missing.It has a register which has a register class. It's true that LLVM currently uses one register class for both scalar and vector uses of XMM registers on x86, but you could argue that that's a problem which should be fixed for other reasons anyway -- it may allow the use of subreg operators to model referencing the first element of a vector, which would allow the coalescer to coalesce them.> That decomposed addressing stuff is interesting, because if one of > your addressing compoenent is a vector, you have a gather-scatter > situation.> >> What is the expected use case for "vector" operands that are not >> registers? What do you plan to use this information for? > > Well, as I explained earlier, I wanted to add type information to > MachinbeMemOperands so I could comment spills as either Vector or > Scalar. That's less important now, so that's why I decided to > drop that for the time being. I still think type information in > the MachineMemOperand is a good idea because it preserves useful > information longer. But I'll come back to that if/when I have > a stronger need. > > As for this patch, which marks instructions and operands as vector, > I don't have an immediate use other than it originally drove the > vector/scalar spill marking before Evan (quite rightly) asked for > a more general approach to deciding if an instruction is a vector > operation. At that point I decided that the MachineMemOperand needed > to change to mark spills. > > In the future, I could imagine this being useful for doing peeps > and other things where we might like to know the cost of an instruction, > analyze the % of a function vectorized, etc.The cost of an instruction depends on the opcode; Vector vs Scalar isn't meaningful there. % of a function vectorized is a rough heuristic at best, and we can't afford to carry around every bit of information that someone might theoretically use for a rough heuristic some day.> > For now, I just don't want to lose this work. Even just applying this > and immediately reverting it is better than nothing because it will > at least be preserved in the repository history.Please don't abuse the repository like this. Dan
On Tuesday 08 December 2009 16:01, Dan Gohman wrote:> > I don't see how a register MachineOperand is obviously a vector unless > > there's type information somewhere that I'm missing. > > It has a register which has a register class. It's true that LLVM > currently uses one register class for both scalar and vector uses > of XMM registers on x86, but you could argue that that's a problem > which should be fixed for other reasons anyway -- it may allow > the use of subreg operators to model referencing the first element > of a vector, which would allow the coalescer to coalesce them.But until that happens, there is no alternative to the patch I submitted.> > For now, I just don't want to lose this work. Even just applying this > > and immediately reverting it is better than nothing because it will > > at least be preserved in the repository history. > > Please don't abuse the repository like this.So the answer is...no? It's a bit frustrating to be told to rework a patch to operate in a different (and better) fashion only to have it rejected. -Dave