On Thu, Jul 31, 2014 at 12:28 AM, Adam Nemet <anemet at apple.com> wrote:> > On Jul 30, 2014, at 10:56 PM, Pete Cooper <peter_cooper at apple.com> wrote: > > Hi Adam > > On Jul 30, 2014, at 10:28 PM, Adam Nemet <anemet at apple.com> wrote: > > Hi Pete, > > Just to clarify, are you proposing two things here? First, 0b… literals > to have type bits<n> and second to allow bits<n> initializer to contain > other bits<m> elements which would initialize the next m elements. > > Yeah, exactly those 2 things. I have them in separate patches, but I > think we only get the benefit from sized binary literals if we also allow > them to initialize multiple bits in another bits<n> type. > > > Looks like bits<n> is already valid in a bits initializer context; it > yields the bottom bit. > > def a { > bits<2> opc = { 0, 1 }; > bits<2> opc2 = { 1, 0 }; > bits<2> oo = { opc, opc2 }; > } > > is valid and produces: > > .. > bits<2> oo = { 1, 0 }; > .. > > Are you aware of this? This may lead to some ambiguity with your proposed > extension. (I have no idea whether this behavior is relied on anywhere > though.) >I think it would make sense to change this behavior to follow the behavior that Pete describes. I don't have the code handy but my guess is that what the code is doing here is requesting each element in the braces of a bits<n> initializer to be casted to `bit`; in that case, we would want to change that code to first try casting to bits and give that preference (and issues relevant diagnostics). Pete, is that what your patch does? Can you attach your patches? Of course, before changing the behavior, we would probably want to place an assert on that codepath to catch all existing cases where the behavior would change. Also it is important to scan all the docs in docs/TableGen/ and make any necessary updates (if there is nowhere that needs to be updated, then documentation needs to be *added* covering this behavior). Finally, we should post a notice to LLVMdev for out-of-tree maintainers including information about how to add the necessary assert to check for places where this behavior would change their .td files. -- Sean Silva> > Adam > > > Thanks, > Pete > > I.e. I don’t think we currently accept: > > bits<4> x = { opc, opc } > > Thanks, > Adam > > > > > _______________________________________________ > 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/20140731/797f0325/attachment.html>
On Jul 31, 2014, at 9:02 AM, Sean Silva <chisophugis at gmail.com> wrote:> I think it would make sense to change this behavior to follow the behavior that Pete describes. I don't have the code handy but my guess is that what the code is doing here is requesting each element in the braces of a bits<n> initializer to be casted to `bit`; in that case, we would want to change that code to first try casting to bits and give that preference (and issues relevant diagnostics). Pete, is that what your patch does? Can you attach your patches?Yeah, thats exactly it. Here’s a WIP patch which fixes that particular issue. I’m building the rest of the compiler now to see if anything breaks with that applied.> > Of course, before changing the behavior, we would probably want to place an assert on that codepath to catch all existing cases where the behavior would change.Sounds like a good plan. I’ll try that and see what happens.> Also it is important to scan all the docs in docs/TableGen/ and make any necessary updates (if there is nowhere that needs to be updated, then documentation needs to be *added* covering this behavior).I haven’t done that yet, but good point. Depending on what changes you all think are acceptable, i’ll update the documentation appropriately.> Finally, we should post a notice to LLVMdev for out-of-tree maintainers including information about how to add the necessary assert to check for places where this behavior would change their .td files.Sure thing. And here’s the other 2 patches. The first sizes binary literals. The second allows them to initialize multiple bits inside { }. Note that i’m only handling certain cases inside the { } initializer. I’m happy to refactor the code and handle other cases if necessary. Thanks, Pete> > -- Sean Silva-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/9b074069/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: var-bits.diff Type: application/octet-stream Size: 1233 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/9b074069/attachment.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/9b074069/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Add-a-new-BinaryIntInit-class-to-tablegen.patch Type: application/octet-stream Size: 13797 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/9b074069/attachment-0001.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/9b074069/attachment-0002.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: 0002-Change-the-expression-in-tablegen-to-accept-sized-bi.patch Type: application/octet-stream Size: 3540 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/9b074069/attachment-0002.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/9b074069/attachment-0003.html>
On Thu, Jul 31, 2014 at 10:08 AM, Pete Cooper <peter_cooper at apple.com> wrote:> > On Jul 31, 2014, at 9:02 AM, Sean Silva <chisophugis at gmail.com> wrote: > > I think it would make sense to change this behavior to follow the behavior > that Pete describes. I don't have the code handy but my guess is that what > the code is doing here is requesting each element in the braces of a > bits<n> initializer to be casted to `bit`; in that case, we would want to > change that code to first try casting to bits and give that preference (and > issues relevant diagnostics). Pete, is that what your patch does? Can you > attach your patches? > > Yeah, thats exactly it. Here’s a WIP patch which fixes that particular > issue. I’m building the rest of the compiler now to see if anything breaks > with that applied. > > > > > Of course, before changing the behavior, we would probably want to place > an assert on that codepath to catch all existing cases where the behavior > would change. > > Sounds like a good plan. I’ll try that and see what happens. > > Also it is important to scan all the docs in docs/TableGen/ and make any > necessary updates (if there is nowhere that needs to be updated, then > documentation needs to be *added* covering this behavior). > > I haven’t done that yet, but good point. Depending on what changes you > all think are acceptable, i’ll update the documentation appropriately. > > Finally, we should post a notice to LLVMdev for out-of-tree maintainers > including information about how to add the necessary assert to check for > places where this behavior would change their .td files. > > Sure thing. > > And here’s the other 2 patches. The first sizes binary literals. >Regarding this patch, does it need to create a whole new Init class? I would try to avoid that if at all possible. Could we just have 0b0111 be a shorthand for a bits<4> BitsInit? Or maybe store some extra data in IntInit?> The second allows them to initialize multiple bits inside { }. Note that > i’m only handling certain cases inside the { } initializer. I’m happy to > refactor the code and handle other cases if necessary. >I don't like the special casing going on in this patch. Could we just switch BitsInit to store pointers to *both* BitInit and BitsInit (it actually can already sort of do that since it has a std::vector<Init*>)? I just feel like this is adding a lot of complexity to TableGen where the behavior that you want could largely be accomplished by removing complexity. -- Sean Silva> > > > > Thanks, > Pete > > > -- Sean Silva > > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140731/907a9301/attachment.html>