On Thu, May 7, 2020 at 6:29 PM Nick Desaulniers ndesaulniers@google.com wrote:
On Thu, May 7, 2020 at 12:19 PM Nick Desaulniers ndesaulniers@google.com wrote:
On Thu, May 7, 2020 at 7:00 AM Brian Gerst brgerst@gmail.com wrote:
This change will make sparse happy and allow these cleanups: #define CONST_MASK(nr) ((u8)1 << ((nr) & 7))
yep, this is more elegant, IMO. Will send a v3 later with this change. Looking at the uses of CONST_MASK, I noticed arch_change_bit() currently has the (u8) cast from commit 838e8bb71dc0c ("x86: Implement change_bit with immediate operand as "lock xorb""), so that instance can get cleaned up with the above suggestion.
Oh, we need the cast to be the final operation. The binary AND and XOR in 2 of the 3 uses of CONST_MASK implicitly promote the operands of the binary operand to int, so the type of the evaluated subexpression is int. https://wiki.sei.cmu.edu/confluence/display/c/EXP14-C.+Beware+of+integer+pro... So I think this version (v2) is most precise fix, and would be better than defining more macros or (worse) using metaprogramming.
One last suggestion. Add the "b" modifier to the mask operand: "orb %b1, %0". That forces the compiler to use the 8-bit register name instead of trying to deduce the width from the input.
-- Brian Gerst