| Message ID | 20200311180416.6509-3-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: gcc asm flag outputs | expand |
On 11/03/2020 6:04 pm, Richard Henderson wrote: > With zero change of behavior, use %[] syntax for the asm > operands instead of numbered operands. For any particular reason? It's very deliberate that a mere 4 instructions have over twice as many lines of comments here, and IMO making the code more verbose only serves to distract from the explanation of what's actually happening. In particular, the value represented by %0 (the conceptual X') never corresponds to the variable "addr", so naming it "addr" provides the opposite of clarity. Robin. > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > arch/arm64/include/asm/uaccess.h | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 32fc8061aa76..ceb1d79eab09 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -75,19 +75,21 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > asm volatile( > // A + B <= C + 1 for all A,B,C, in four easy steps: > // 1: X = A + B; X' = X % 2^64 > - " adds %0, %3, %2\n" > + " adds %[addr], %[addr], %[size]\n" > // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4 > - " csel %1, xzr, %1, hi\n" > + " csel %[limit], xzr, %[limit], hi\n" > // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X' > // to compensate for the carry flag being set in step 4. For > // X > 2^64, X' merely has to remain nonzero, which it does. > - " csinv %0, %0, xzr, cc\n" > + " csinv %[addr], %[addr], xzr, cc\n" > // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1 > // comes from the carry in being clear. Otherwise, we are > // testing X' - C == 0, subject to the previous adjustments. > - " sbcs xzr, %0, %1\n" > - " cset %0, ls\n" > - : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc"); > + " sbcs xzr, %[addr], %[limit]\n" > + " cset %[addr], ls\n" > + : [addr] "=&r" (ret), [limit] "+r" (limit) > + : [size] "Ir" (size), "0" (addr) > + : "cc"); > > return ret; > } >
On 3/11/20 12:08 PM, Robin Murphy wrote: > On 11/03/2020 6:04 pm, Richard Henderson wrote: >> With zero change of behavior, use %[] syntax for the asm >> operands instead of numbered operands. > > For any particular reason? When we get to the third patch and add CC_SET(le), all of the operand numbers will change, and I found that more confusing than not. > In > particular, the value represented by %0 (the conceptual X') never corresponds > to the variable "addr", so naming it "addr" provides the opposite of clarity. Would you simply prefer a different name for the operand? r~ >> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h >> index 32fc8061aa76..ceb1d79eab09 100644 >> --- a/arch/arm64/include/asm/uaccess.h >> +++ b/arch/arm64/include/asm/uaccess.h >> @@ -75,19 +75,21 @@ static inline unsigned long __range_ok(const void __user >> *addr, unsigned long si >> asm volatile( >> // A + B <= C + 1 for all A,B,C, in four easy steps: >> // 1: X = A + B; X' = X % 2^64 >> - " adds %0, %3, %2\n" >> + " adds %[addr], %[addr], %[size]\n" >> // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4 >> - " csel %1, xzr, %1, hi\n" >> + " csel %[limit], xzr, %[limit], hi\n" >> // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X' >> // to compensate for the carry flag being set in step 4. For >> // X > 2^64, X' merely has to remain nonzero, which it does. >> - " csinv %0, %0, xzr, cc\n" >> + " csinv %[addr], %[addr], xzr, cc\n" >> // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1 >> // comes from the carry in being clear. Otherwise, we are >> // testing X' - C == 0, subject to the previous adjustments. >> - " sbcs xzr, %0, %1\n" >> - " cset %0, ls\n" >> - : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc"); >> + " sbcs xzr, %[addr], %[limit]\n" >> + " cset %[addr], ls\n" >> + : [addr] "=&r" (ret), [limit] "+r" (limit) >> + : [size] "Ir" (size), "0" (addr) >> + : "cc"); >> return ret; >> }
On 2020-03-11 9:48 pm, Richard Henderson wrote: > On 3/11/20 12:08 PM, Robin Murphy wrote: >> On 11/03/2020 6:04 pm, Richard Henderson wrote: >>> With zero change of behavior, use %[] syntax for the asm >>> operands instead of numbered operands. >> >> For any particular reason? > > When we get to the third patch and add CC_SET(le), all of the operand numbers > will change, and I found that more confusing than not. > >> In >> particular, the value represented by %0 (the conceptual X') never corresponds >> to the variable "addr", so naming it "addr" provides the opposite of clarity. > > Would you simply prefer a different name for the operand? If we were to go down this route, I think it might actually make sense to split it up further to separate the "private scratch register" and "return value" concerns. Robin. > > > r~ > > >>> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h >>> index 32fc8061aa76..ceb1d79eab09 100644 >>> --- a/arch/arm64/include/asm/uaccess.h >>> +++ b/arch/arm64/include/asm/uaccess.h >>> @@ -75,19 +75,21 @@ static inline unsigned long __range_ok(const void __user >>> *addr, unsigned long si >>> asm volatile( >>> // A + B <= C + 1 for all A,B,C, in four easy steps: >>> // 1: X = A + B; X' = X % 2^64 >>> - " adds %0, %3, %2\n" >>> + " adds %[addr], %[addr], %[size]\n" >>> // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4 >>> - " csel %1, xzr, %1, hi\n" >>> + " csel %[limit], xzr, %[limit], hi\n" >>> // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X' >>> // to compensate for the carry flag being set in step 4. For >>> // X > 2^64, X' merely has to remain nonzero, which it does. >>> - " csinv %0, %0, xzr, cc\n" >>> + " csinv %[addr], %[addr], xzr, cc\n" >>> // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1 >>> // comes from the carry in being clear. Otherwise, we are >>> // testing X' - C == 0, subject to the previous adjustments. >>> - " sbcs xzr, %0, %1\n" >>> - " cset %0, ls\n" >>> - : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc"); >>> + " sbcs xzr, %[addr], %[limit]\n" >>> + " cset %[addr], ls\n" >>> + : [addr] "=&r" (ret), [limit] "+r" (limit) >>> + : [size] "Ir" (size), "0" (addr) >>> + : "cc"); >>> return ret; >>> }
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 32fc8061aa76..ceb1d79eab09 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -75,19 +75,21 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si asm volatile( // A + B <= C + 1 for all A,B,C, in four easy steps: // 1: X = A + B; X' = X % 2^64 - " adds %0, %3, %2\n" + " adds %[addr], %[addr], %[size]\n" // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4 - " csel %1, xzr, %1, hi\n" + " csel %[limit], xzr, %[limit], hi\n" // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X' // to compensate for the carry flag being set in step 4. For // X > 2^64, X' merely has to remain nonzero, which it does. - " csinv %0, %0, xzr, cc\n" + " csinv %[addr], %[addr], xzr, cc\n" // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1 // comes from the carry in being clear. Otherwise, we are // testing X' - C == 0, subject to the previous adjustments. - " sbcs xzr, %0, %1\n" - " cset %0, ls\n" - : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc"); + " sbcs xzr, %[addr], %[limit]\n" + " cset %[addr], ls\n" + : [addr] "=&r" (ret), [limit] "+r" (limit) + : [size] "Ir" (size), "0" (addr) + : "cc"); return ret; }
With zero change of behavior, use %[] syntax for the asm operands instead of numbered operands. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- arch/arm64/include/asm/uaccess.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)