<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">LGTM, too. Please commit. Thanks, guys.<div><br></div><div>-Jim</div><div><br><div><blockquote type="cite"><div>On Aug 29, 2014, at 10:24 AM, Pete Cooper <<a href="mailto:peter_cooper@apple.com">peter_cooper@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Oops, this was clearly something I’d meant to support in my original sequence of patches.<div class=""><br class=""></div><div class="">LGTM.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Pete<br class=""><div><blockquote type="cite" class=""><div class="">On Aug 29, 2014, at 10:18 AM, Jean-Luc Duprat <<a href="mailto:jduprat@apple.com" class="">jduprat@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=windows-1252" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">In trying to use Pete’s new Tablegen syntax, I ran into a minor issue with initializing bits from variables.<div class="">This patch addresses the issue.<div class=""><br class=""></div><div class=""></div></div></div><span id="cid:F040F1AC-5CBE-4FD6-93D3-31553A926D02@apple.com"><tblgen.diff></span><meta http-equiv="Content-Type" content="text/html charset=windows-1252" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""><br class=""><div class=""><div class=""><div class=""><br class=""></div><div class=""><br class=""></div><div class="">JL</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">On Aug 6, 2014, at 18:15 , Pete Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><meta http-equiv="Content-Type" content="text/html charset=windows-1252" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><div class="">On Aug 6, 2014, at 5:39 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" class="">chisophugis@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><div dir="ltr" class="">LGTM with some nits.<div class=""><br class=""></div><div class="">Please commit 0005 first, with a mention in the commit message to the effect that this cleans things up for the upcoming change, also state the rationale.</div></div></blockquote>Sure, sounds good.<br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class=""><br class="">
</div><div class="">Also, 0006 contains a random change to test/TableGen/BitsInit.td which should be incorporated into one of the previous commits (or as a separate commit), so that 0006 can be just docs changes.</div></div></blockquote>Oops, my bad. I merged it with the wrong commit. That should go in with 0004. I’ll change that.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Pete<br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class=""><br class=""></div>
<div class="">-- Sean Silva</div></div><div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On Wed, Aug 6, 2014 at 4:42 PM, Pete Cooper <span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">Hi Sean<div class=""><br class=""></div><div class="">I’ve updated the patches and included the test for bits<> assigned to int. </div>
<div class=""><br class=""></div><div class="">I’ve also found that we should only be allowing !if expressions to be converted to bit values when both sides of the !if are single bits. This is also an improvement over today where we’d silently drop bits. I’ve added a test for this too.</div>
<div class=""><br class=""></div><div class="">Please let me know if you have any other comments.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">Pete</div><div class=""><br class=""></div><div class=""></div></div><br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><br class=""><div class=""><div class="">On Jul 31, 2014, at 4:26 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank" class="">chisophugis@gmail.com</a>> wrote:</div><br class=""><blockquote type="cite" class="">
<div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On Thu, Jul 31, 2014 at 5:02 PM, Pete Cooper <span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><div class=""><div class="">On Jul 31, 2014, at 3:03 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank" class="">chisophugis@gmail.com</a>> wrote:</div>
<br class=""><blockquote type="cite" class=""><div dir="ltr" class="">Did you see anywhere in the docs that needed updating?</div></blockquote></div>Yeah, here’s what I’ve got so far. I’m not 100% sure on the how well i’ve organized the changes in LangRef.rst, but I think its close to explaining that we can now nest { } values.</div>
<div class=""><br class=""></div></div></blockquote><div class=""><br class=""></div><div class=""><br class=""></div><div class="">I don't think that your patches actually change the acceptable syntax so I don't think that your change to LangRef.rst is needed.</div><div class="">However, you should add a sentence following the BinInteger production saying that it is treated as an object of type "bits<n>" (where n = number of given bits) and not "int", but will still convert to an int if needed. Now that I think about it, could you make sure that we have test coverage for a 0bxx literal initializing an "int"?</div>
<div class=""><br class=""></div><div class="">The LangIntro changes LGTM.</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""></div><div class=""></div>
</div><br class=""><div style="word-wrap:break-word" class=""><div class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">Regarding patch 0005, thankfully those were benign since they just had leading 0's. Can you doublecheck the diagnostic quality? We need to give a clear indication that what is wrong so that users don't get left scratching their heads.</div>
</div></blockquote>I added a better error message in r214374. As an example from those that were fixed in patch 5, this is what I saw before I made a fix:</div><div class=""><br class=""></div><div class=""><div style="margin:0px;font-size:11px;font-family:Menlo" class="">
lib/Target/Sparc/SparcInstrVIS.td:260:16: error: Value 'VISInst:opfval' of type 'bits<9>' is incompatible with initializer '{ 0, 0, 0, 0, 0, 1, 0, 1, 1, 0 }' of type bit initializer with length 10'</div>
<div style="margin:0px;font-size:11px;font-family:Menlo" class="">def UMULXHI : VISInst<0b0000010110, "umulxhi", I64Regs>;</div><div style="margin:0px;font-size:11px;font-family:Menlo" class=""> ^</div><blockquote type="cite" class="">
<div dir="ltr" class="">
<div class=""><br class=""></div></div></blockquote></div></div></blockquote><div class="">That seems sane.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">
<div class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class=""></div><div class=""><br class=""></div><div class=""><div class="">+ /// resolveListElementReference - This method is used to implement</div><div class="">+ /// VarListElementInit::resolveReferences. If the list element is resolvable</div>
<div class="">+ /// now, we return the resolved value, otherwise we return null.</div>
<div class="">+ Init *resolveListElementReference(Record &R, const RecordVal *RV,</div><div class="">+ unsigned Elt) const override {</div><div class="">+ llvm_unreachable("Illegal element reference off bits<n>");</div>
<div class="">+ }</div></div><div class=""><br class=""></div><div class="">This saddens me... all this TableGen code really needs to be refactored...</div><div class="">There may be a simple refactor for this resolveListElementReference situation that does not require plunging into the murky depths of the resolution machinery.</div>
</div></blockquote>I’m open to suggestions :) Otherwise, is there anything you can think of thats better than having BitsInit now being a TypedInit subclass?</div></div></blockquote><div class=""><br class=""></div><div class="">Mostly I was wondering if there are sufficiently few places that call resolveListElementReference that we could just pull the entire logic for resolveListElementReference out of these classes entirely and make it a static function in the relevant .cpp file.</div>
<div class=""><br class=""></div><div class="">-- Sean Silva</div><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><br class=""></div><div class="">Thanks,</div>
<div class="">
Pete<br class=""><blockquote type="cite" class=""><div dir="ltr" class="">
<div class=""><br class=""></div><div class=""><br class=""></div><div class="">-- Sean Silva</div><div class=""><br class=""></div></div><div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On Thu, Jul 31, 2014 at 2:48 PM, Pete Cooper <span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">Hi all<div class=""><br class=""></div><div class="">Moving this from LLVMDev (<a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/075223.html" target="_blank" class="">http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/075223.html</a>) now that i have patches i’d welcome reviews of. The following is the initial summary from that email thread.</div>
<div class=""><br class=""></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px" class=""><div class="">"Currently tablegen parses binary literals such as 0b011 and immediately turns them in to integers internally. Due to this, 0b011 is a 2-bit value because tablegen will happily drop leading zeroes on integers.</div>
<div class=""><br class=""></div><div class="">I propose that we change this, and store binary literals with a size. I think this is much more natural, as when the user writes a value with 3 bits, I think they expect to get a value with 3 bits, and not a 2-bit value which is silently truncated and later silently zero extended.</div>
<div class=""><br class=""></div><div class="">Given this, I would then like to extend bits initializers to accept these new sized binary literals. For example, I would like to be able to write</div><div class=""><br class=""></div><div class="">bits<2> opc;</div><div class="">
bits<8> x = { opc, 0b1100, rd{1}, rs{0} }</div><div class=""><br class=""></div><div class="">This would let us write some encodings much more concisely, instead of having to put each initializer on its own line.”</div></blockquote><div class=""><br class="">
</div>
<div class="">For those interested in the low level details, i’ve changed binary literals such as 0b011 from creating an integer ‘3’, to creating a BitsInit ‘{ 0, 1, 1}’. As Sean suggested, this was better than introducing yet another type just for binary literals.</div>
<div class=""><br class=""></div><div class="">As part of this, BitsInit values now had to be able to be typed. This is because we had uses of binary literals which expected to be typed (for example list<bits<2>> [0b00, 0b01]). I’ve moved BitsInit from being a subclass of Init to a subclass of TypedInit to allow this.</div>
<div class=""><br class=""></div><div class="">Note that patches 2 and 3 really need to be committed together. I can squash them before commit, but just wanted to present the changes independently.</div><div class=""><br class=""></div><div class="">Patch 5 can also be committed without any of the others, but just shows that this did actually catch some useful cases where we mismatched sizes of binary expressions.</div>
<div class=""><br class=""></div><div class="">Thanks,</div><div class="">Pete</div><div class=""><br class=""></div><div class=""></div></div><br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""></div></div>
<br class=""></blockquote></div><br class=""></div>
</blockquote></div><br class=""></div><br class=""></blockquote></div><br class=""></div></div>
</blockquote></div><br class=""></div><br class=""></blockquote></div><br class=""></div>
</blockquote></div><br class=""></div>_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class=""></blockquote></div><br class=""></div></div></div></div></div></blockquote></div><br class=""></div></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></div></blockquote></div><br></div></body></html>