Segher Boessenkool <seg...@kernel.crashing.org> writes: > Hi! > > On Fri, Apr 16, 2021 at 05:44:52PM +1000, Daniel Axtens wrote: >> Sathvika Vasireddy <sathv...@linux.vnet.ibm.com> writes: >> Ok, if I've understood correctly... >> >> > + ra = ra & ~0x3; >> >> This masks off the bits of RA that are not part of BTF: >> >> ra is in [0, 31] which is [0b00000, 0b11111] >> Then ~0x3 = ~0b00011 >> ra = ra & 0b11100 >> >> This gives us then, >> ra = btf << 2; or >> btf = ra >> 2; > > Yes. In effect, you want the offset in bits of the CR field, which is > just fine like this. But a comment would not hurt. > >> Let's then check to see if your calculations read the right fields. >> >> > + if ((regs->ccr) & (1 << (31 - ra))) >> > + op->val = -1; >> > + else if ((regs->ccr) & (1 << (30 - ra))) >> > + op->val = 1; >> > + else >> > + op->val = 0; > > It imo is clearer if written > > if ((regs->ccr << ra) & 0x80000000) > op->val = -1; > else if ((regs->ccr << ra) & 0x40000000) > op->val = 1; > else > op->val = 0; > > but I guess not everyone agrees :-) > >> CR field: 7 6 5 4 3 2 1 0 >> bit: 0123 0123 0123 0123 0123 0123 0123 0123 >> normal bit #: 0.....................................31 >> ibm bit #: 31.....................................0 > > The bit numbers in CR fields are *always* numbered left-to-right. I > have never seen anyone use LE for it, anyway. > > Also, even people who write LE have the bigger end on the left normally > (they just write some things right-to-left, and other things > left-to-right).
Sorry, the numbers in the CR fields weren't meant to be especially meaningful, I was just trying to convince myself that we referenced the same bits doing the ISA way vs the way this code did it. Kind regards, Daniel > >> Checkpatch does have one complaint: >> >> CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'regs->ccr' >> #30: FILE: arch/powerpc/lib/sstep.c:1971: >> + if ((regs->ccr) & (1 << (31 - ra))) >> >> I don't really mind the parenteses: I think you are safe to ignore >> checkpatch here unless someone else complains :) > > I find them annoying. If there are too many parentheses, it is hard to > see at a glance what groups where. Also, a suspicious reader might > think there is something special going on (with macros for example). > > This is simple code of course, but :-) > >> If you do end up respinning the patch, I think it would be good to make >> the maths a bit clearer. I think it works because a left shift of 2 is >> the same as multiplying by 4, but it would be easier to follow if you >> used a temporary variable for btf. > > It is very simple. The BFA instruction field is closely related to the > BI instruction field, which is 5 bits, and selects one of the 32 bits in > the CR. If you have "BFA00 BFA01 BFA10 BFA11", that gives the bit > numbers of all four bits in the selected CR field. So the "& ~3" does > all you need. It is quite pretty :-) > > > Segher
