<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;"><div><br><div><div>On Jul 31, 2014, at 4:26 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 31, 2014 at 5:02 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class=""><div>On Jul 31, 2014, at 3:03 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">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><br></div></div></blockquote><div><br></div><div><br></div><div>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>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><br></div><div>The LangIntro changes LGTM.</div><div> </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"><div></div><div></div>
</div><br><div style="word-wrap:break-word"><div><blockquote type="cite"><div dir="ltr"><div><br></div><div>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><br></div><div><div style="margin:0px;font-size:11px;font-family:Menlo">
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">def UMULXHI : VISInst<0b0000010110, "umulxhi", I64Regs>;</div><div style="margin:0px;font-size:11px;font-family:Menlo"> ^</div><blockquote type="cite">
<div dir="ltr">
<div><br></div></div></blockquote></div></div></blockquote><div>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">
<div><blockquote type="cite"><div dir="ltr"><div></div><div><br></div><div><div>+ /// resolveListElementReference - This method is used to implement</div><div>+ /// VarListElementInit::resolveReferences. If the list element is resolvable</div>
<div>+ /// now, we return the resolved value, otherwise we return null.</div>
<div>+ Init *resolveListElementReference(Record &R, const RecordVal *RV,</div><div>+ unsigned Elt) const override {</div><div>+ llvm_unreachable("Illegal element reference off bits<n>");</div>
<div>+ }</div></div><div><br></div><div>This saddens me... all this TableGen code really needs to be refactored...</div><div>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><br></div><div>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><br></div><div>-- Sean Silva</div><div> </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"><div><br></div><div>Thanks,</div><div>
Pete<br><blockquote type="cite"><div dir="ltr">
<div><br></div><div><br></div><div>-- Sean Silva</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 31, 2014 at 2:48 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi all<div><br></div><div>Moving this from LLVMDev (<a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/075223.html" target="_blank">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><br></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div>"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><br></div><div>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><br></div><div>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><br></div><div>bits<2> opc;</div><div>
bits<8> x = { opc, 0b1100, rd{1}, rs{0} }</div><div><br></div><div>This would let us write some encodings much more concisely, instead of having to put each initializer on its own line.”</div></blockquote><div><br>
</div>
<div>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><br></div><div>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><br></div><div>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><br></div><div>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><br></div><div>Thanks,</div><div>Pete</div><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div></div>
<br><div style="word-wrap:break-word"><div></div></div>
<br><div style="word-wrap:break-word"><div></div></div>
<br><div style="word-wrap:break-word"><div></div></div>
<br><div style="word-wrap:break-word"><div></div></div>
<br></blockquote></div><br></div>
</blockquote></div><br></div><br></blockquote></div><br></div></div>
</blockquote></div><br></div></body></html>