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
On Dec 9, 2009, at 8:20 AM, David Greene wrote:> 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.True, but there isn't anything using this information (specifically, isVector flags on operands) anyway, unless there's something we don't know about.>>> 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?My comment here was in response to your comment here.> > 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.I've not followed closely enough to know who told you to do it this way; if they think the new patch is good then that's fine with me. It looks like it probably won't break anything. It's still very unclear what's motivating this patch, except that you seem to need it for your own internal reasons, and it's inconvenient to maintain it in your own tree. There are precedents for this kind of thing though. Dan
On Wednesday 09 December 2009 18:33, Dan Gohman wrote:> > But until that happens, there is no alternative to the patch I submitted. > > True, but there isn't anything using this information > (specifically, isVector flags on operands) anyway, unless > there's something we don't know about.No, nothing currently.> > 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. > > I've not followed closely enough to know who told you to do it > this way; if they think the new patch is good then that's fine > with me. It looks like it probably won't break anything.Evan asked me to rework it. I'm glad he did, for I learned a few new TableGen tricks. So the exercise wasn't a complete waste. I've decided I'm going to file the patch away in a little "to-do" folder and when I need it I can pull it back out. Until then, I don't see a reason to push it upstream if no one else finds it especially useful at the moment. It's more important I get other stuff merged anyway, so I don't think we need to do anything more with this right now. -Dave