On Oct 6, 2011, at 7:59 AM, David A. Greene wrote:> For example, I want to be able to do this: > > defm MOVH : > vs1x_fps_binary_vv_node_rmonly< > 0x16, "movh", undef, 0, > // rr > [(undef)], > // rm > [(set DSTREGCLASS:$dst, > (DSTTYPE (movlhps SRCREGCLASS:$src1, > (DSTTYPE (bitconvert > (v2f64 (scalar_to_vector > (loadf64 addr:$src2))))))))], > // rr Pat > [], > // rm Pat > [[(DSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), > (MNEMONIC SRCREGCLASS:$src1, addr:$src2)], > [(INTDSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), > (MNEMONIC SRCREGCLASS:$src1, addr:$src2)]]>;This kind of thing is very hard to read and understand. ISAs don't change a lot, and .td files are read lot more than they are written. I actually think it is preferable to leave some redundancy in the .td files, it makes it much easier to find things. It is already tedious to expand multiclasses in your head when you are looking for the properties of a specific instruction. /jakob
On Oct 6, 2011, at 11:12 AM, Jakob Stoklund Olesen wrote:> > On Oct 6, 2011, at 7:59 AM, David A. Greene wrote: > >> For example, I want to be able to do this: >> >> defm MOVH : >> vs1x_fps_binary_vv_node_rmonly< >> 0x16, "movh", undef, 0, >> // rr >> [(undef)], >> // rm >> [(set DSTREGCLASS:$dst, >> (DSTTYPE (movlhps SRCREGCLASS:$src1, >> (DSTTYPE (bitconvert >> (v2f64 (scalar_to_vector >> (loadf64 addr:$src2))))))))], >> // rr Pat >> [], >> // rm Pat >> [[(DSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), >> (MNEMONIC SRCREGCLASS:$src1, addr:$src2)], >> [(INTDSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), >> (MNEMONIC SRCREGCLASS:$src1, addr:$src2)]]>; > > This kind of thing is very hard to read and understand. > > ISAs don't change a lot, and .td files are read lot more than they are written. I actually think it is preferable to leave some redundancy in the .td files, it makes it much easier to find things. > > It is already tedious to expand multiclasses in your head when you are looking for the properties of a specific instruction.This is a critical point. The readability of the .td files directly relates to their maintainability, and the ease of access for newer contributors. -Jim
Jakob Stoklund Olesen <jolesen at apple.com> writes:> On Oct 6, 2011, at 7:59 AM, David A. Greene wrote: > >> For example, I want to be able to do this: >> >> defm MOVH : >> vs1x_fps_binary_vv_node_rmonly< >> 0x16, "movh", undef, 0, >> // rr >> [(undef)], >> // rm >> [(set DSTREGCLASS:$dst, >> (DSTTYPE (movlhps SRCREGCLASS:$src1, >> (DSTTYPE (bitconvert >> (v2f64 (scalar_to_vector >> (loadf64 addr:$src2))))))))], >> // rr Pat >> [], >> // rm Pat >> [[(DSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), >> (MNEMONIC SRCREGCLASS:$src1, addr:$src2)], >> [(INTDSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), >> (MNEMONIC SRCREGCLASS:$src1, addr:$src2)]]>; > > This kind of thing is very hard to read and understand.What's hard about it? I'm not trying to be agitational here. I'm truly wondering what I can do to make this more understandable.> ISAs don't change a lotWell, they do on some common architecutres. :)> .td files are read lot more than they are written. I actually think > it is preferable to leave some redundancy in the .td files, it makes > it much easier to find things.I had two goals when I designed our AVX implementation. Make it easily extensible to longer vector lengths while keeping it as readable as possible. I am certainly happy to make things more readable and welcome lots of feedback in that area. But the ability to quickly and easily extend the ISA for new vector lengths is critical to us. That drives the generic pattern specification above and the !foreach/!subst stuff you see in MultiPat.td. As I explained in another message, I think a lot of MultiPat.td can be cleaned up with some semi-clever uses of the for proposal. I actually find the above easier to read and grasp that the multitude of individual patterns for each different kind of ADD, SUB, etc. because I don't have to go searching to find out how each one is defined. They're all defined exactly the same way. But of course, I've been working with this for a long time. I'm glad to be getting some feedback on it now. :) -Dave
On Oct 6, 2011, at 12:42 PM, David A. Greene wrote:> Jakob Stoklund Olesen <jolesen at apple.com> writes: > >> On Oct 6, 2011, at 7:59 AM, David A. Greene wrote: >> >>> For example, I want to be able to do this: >>> >>> defm MOVH : >>> vs1x_fps_binary_vv_node_rmonly< >>> 0x16, "movh", undef, 0, >>> // rr >>> [(undef)], >>> // rm >>> [(set DSTREGCLASS:$dst, >>> (DSTTYPE (movlhps SRCREGCLASS:$src1, >>> (DSTTYPE (bitconvert >>> (v2f64 (scalar_to_vector >>> (loadf64 addr:$src2))))))))], >>> // rr Pat >>> [], >>> // rm Pat >>> [[(DSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), >>> (MNEMONIC SRCREGCLASS:$src1, addr:$src2)], >>> [(INTDSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), >>> (MNEMONIC SRCREGCLASS:$src1, addr:$src2)]]>; >> >> This kind of thing is very hard to read and understand. > > What's hard about it? I'm not trying to be agitational here. I'm truly > wondering what I can do to make this more understandable.If you didn't write these patterns yourself, or if you wrote them six months ago, it is nearly impossible to figure out where a specific pattern came from, or where a specific instruction is defined. It is hard enough mentally executing the current multiclasses. Injecting patterns into multi defms like this makes it much harder still.> I am certainly happy to make things more readable and welcome > lots of feedback in that area. But the ability to quickly and easily > extend the ISA for new vector lengths is critical to us.This is where our priorities differ. Readability and maintainability are key. After all, we need to fix isel and codegen bugs more often than Intel and AMD add ISA extensions. /jakob
Hi David, In case you didn't see my reply in llvm-commits. Let me repeat my objections here. If I understand correctly, the reason you are pushing for all these extra abstraction features in TableGen language is to one day refactor SSE / AVX. The intention is to support future Intel ISA extensions which may extend AVX to vectors of arbitrary width. Am I understanding the purpose of these patches correctly? Unfortunately I strongly disagree with the direction you are taking with this. If we are to implement support for future Intel vector extension, we should add these instructions separately. I understand you are concerned about code duplication and would prefer to leverage what's already written. However, our experience has shown the extra levels of abstraction is always high level that makes TableGen debugging harder than it should. Adding more layers, as you are proposing, would just make it unnecessary difficult. I believe it will be counterproductive. I have discussed this carefully with Chris and have decided this is not the direction where we want to go with TableGen. We would really appreciate that you would back out these changes. Thanks, Evan On Oct 6, 2011, at 12:42 PM, David A. Greene wrote:> Jakob Stoklund Olesen <jolesen at apple.com> writes: > >> On Oct 6, 2011, at 7:59 AM, David A. Greene wrote: >> >>> For example, I want to be able to do this: >>> >>> defm MOVH : >>> vs1x_fps_binary_vv_node_rmonly< >>> 0x16, "movh", undef, 0, >>> // rr >>> [(undef)], >>> // rm >>> [(set DSTREGCLASS:$dst, >>> (DSTTYPE (movlhps SRCREGCLASS:$src1, >>> (DSTTYPE (bitconvert >>> (v2f64 (scalar_to_vector >>> (loadf64 addr:$src2))))))))], >>> // rr Pat >>> [], >>> // rm Pat >>> [[(DSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), >>> (MNEMONIC SRCREGCLASS:$src1, addr:$src2)], >>> [(INTDSTTYPE (movlhps SRCREGCLASS:$src1, (load addr:$src2))), >>> (MNEMONIC SRCREGCLASS:$src1, addr:$src2)]]>; >> >> This kind of thing is very hard to read and understand. > > What's hard about it? I'm not trying to be agitational here. I'm truly > wondering what I can do to make this more understandable. > >> ISAs don't change a lot > > Well, they do on some common architecutres. :) > >> .td files are read lot more than they are written. I actually think >> it is preferable to leave some redundancy in the .td files, it makes >> it much easier to find things. > > I had two goals when I designed our AVX implementation. Make it easily > extensible to longer vector lengths while keeping it as readable as > possible. I am certainly happy to make things more readable and welcome > lots of feedback in that area. But the ability to quickly and easily > extend the ISA for new vector lengths is critical to us. That drives > the generic pattern specification above and the !foreach/!subst stuff > you see in MultiPat.td. As I explained in another message, I think a > lot of MultiPat.td can be cleaned up with some semi-clever uses of the > for proposal. > > I actually find the above easier to read and grasp that the multitude of > individual patterns for each different kind of ADD, SUB, etc. because I > don't have to go searching to find out how each one is defined. They're > all defined exactly the same way. > > But of course, I've been working with this for a long time. I'm glad to > be getting some feedback on it now. :) > > -Dave > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20111006/6e2a017f/attachment.html>