Hi Paul, Thanks a bunch for the review. On Tue, Sep 18, 2018 at 10:25 PM Paul Burton <paul.burton@xxxxxxxx> wrote: > Should this be .set reorder? Nice catch. Fixed here: https://git.zx2c4.com/WireGuard/commit/?id=23d97fc333cf85dd07445a9d21a28cbef47c553c But then... > Even better - could we not just place the addiu before the bne & drop > the .set noreorder, allowing the assembler to fill the delay slot with > the addiu? Likewise in many other places throughout the patch. > > That would be more future proof - particularly if we ever want to adjust > this for use with the nanoMIPS ISA which has no delay slots. It may also > allow the assembler the choice to use compact branches (ie. branches > without visible delay slots) when targeting MIPS32r6. I know neither of > these will currently build this code, but I think avoiding all the > noreorder blocks would be a nice cleanup just for the sake of > readability anyway. Great idea. Rene has committed that here: https://git.zx2c4.com/WireGuard/commit/?id=5c153a59ac3aa58a3ff17c69fee63d599e5f2758 These will be in the v6 patchset whenever that's posted, and it's already been merged into the dev tree: https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/log/?h=jd/wireguard Regards, Jason
