I submitted a series of patches to convert all uses of log2 values to non-log2 values (That was harder than I thought because the types of the two are the same. They only different in meaning. So it was not easy to distinguish them. I split up patches so that anyone can verify correctness of the conversion that I've done.)>From now on, please always use non-log2 alignment values exclusively inLLD. Thanks! On Wed, Mar 25, 2015 at 12:16 PM, Sean Silva <chisophugis at gmail.com> wrote:> > > On Tue, Mar 24, 2015 at 5:09 PM, Rui Ueyama <ruiu at google.com> wrote: > >> It's not a big deal, but it always annoyed me a bit when I hit it, so >> I'll bring it up here. >> >> LLD represents an alignment X as log2(X) in some places and just X in >> other places. It's a bit confusing. Because I always think alignments in my >> mind in terms of 1, 2, 4, 8, ..., instead of 2^1, 2^2, 2^3, ..., I'd like >> to propose to always use real values. >> >> Any objections? >> > > The only theoretical objection I can imagine is that you can use much less > storage for holding the log2(X). But right now that isn't causing us any > problems it seems. > > -- Sean Silva > > >> >> _______________________________________________ >> 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/20150325/1aefca22/attachment.html>
Shankar Easwaran
2015-Mar-26 16:23 UTC
[LLVMdev] LLD: representation of a power of two value
I was expecting that you would send the patches for review prior committing the patch. We could follow this style for all future commits that things get reviewed before committing, so that its easier to follow a uniform policy. For example you added a PowerOf2 class in DefinedAtom, for a simple reason that the class usage went away :) I thought it would also have helped if you made few commits to cleanup, which IMO would be easier to review and get a overall sense of the code. Shankar Easwaran On 3/25/2015 9:30 PM, Rui Ueyama wrote:> I submitted a series of patches to convert all uses of log2 values to > non-log2 values (That was harder than I thought because the types of the > two are the same. They only different in meaning. So it was not easy to > distinguish them. I split up patches so that anyone can verify correctness > of the conversion that I've done.) > > >From now on, please always use non-log2 alignment values exclusively in > LLD. Thanks! > > On Wed, Mar 25, 2015 at 12:16 PM, Sean Silva <chisophugis at gmail.com> wrote: >-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation
I requested you pre-commit code review several times recently because you submitted them without any discussion or code review. This is not the case. Also please note that the patches looked actually dubious. As the history of this project shows, I'm probably the person who cares most about the LLD's code healthiness, and probably the person who contributed to that area most. I'm sorry to say that, but your coding practice sometimes seem very odd to me (e.g. being against even removing completely-dead code, or doing something too fancy with several levels of indirections), so I need to review your code carefully. As to the patches, I usually don't split a refactoring patch. This is an exception -- this would actually help others understand code. If each step of the patches look trivial, it served its purpose -- everything looks obvious and mechanical translation from the old code to the new one. Had I done that in one large patch, no one wouldn't have been able to verify that I didn't miss something. On Thu, Mar 26, 2015 at 9:23 AM, Shankar Easwaran <shankare at codeaurora.org> wrote:> I was expecting that you would send the patches for review prior > committing the patch. > > We could follow this style for all future commits that things get reviewed > before committing, so that its easier to follow a uniform policy. > > For example you added a PowerOf2 class in DefinedAtom, for a simple reason > that the class usage went away :) I thought it would also have helped if > you made few commits to cleanup, which IMO would be easier to review and > get a overall sense of the code. > > Shankar Easwaran > > On 3/25/2015 9:30 PM, Rui Ueyama wrote: > >> I submitted a series of patches to convert all uses of log2 values to >> non-log2 values (That was harder than I thought because the types of the >> two are the same. They only different in meaning. So it was not easy to >> distinguish them. I split up patches so that anyone can verify correctness >> of the conversion that I've done.) >> >> >From now on, please always use non-log2 alignment values exclusively in >> LLD. Thanks! >> >> On Wed, Mar 25, 2015 at 12:16 PM, Sean Silva <chisophugis at gmail.com> >> wrote: >> >> > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted > by the Linux Foundation > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150326/be8b8bcf/attachment.html>