[cfe-commits] [PATCH][review request] Add support for LLVM intrinsic bswap16

[James Orr]

On Mar 21, 2011, at 1:33 AM, Eli Friedman wrote:

> On Sat, Mar 19, 2011 at 8:02 PM, James Orr <jorr at apple.com> wrote:
>> 
>> On Mar 19, 2011, at 7:25 PM, Chris Lattner wrote:
>> 
>>> 
>>> On Mar 19, 2011, at 3:24 PM, Eli Friedman wrote:
>>> 
>>>> On Sat, Mar 19, 2011 at 3:15 PM, James Orr <jorr at apple.com> wrote:
>>>>> Add support for built in 16bit byte swap.
>>>> 
>>>> Why?
>>> 
>>> Yes, why?  GCC doesn't support this either IIRC.
>>> 
>>> -Chris
>> 
>> I want to convert a big endian short to little endian (ie Cortex-M0 reading over I2C a 16bit ADC value returned in big endian).
>> 
>> Clang does not recognize the following as a byte swap: (uint16_t)( a >> 8) | (uint16_t)( a << 8); (llvm-gcc does)
> 
> Hmm, interesting... that issue actually appears to be ARM-specific.
> Hopefully it shouldn't be too hard to fix up InstCombiner::MatchBSwap
> to do the right thing here.

Yea it looks like other archs with clang do what I would expect:

target triple = "x86_64-apple-darwin10.0.0"

define zeroext i16 @bswapC(i16 zeroext %a) nounwind readnone optsize ssp {
  %1 = tail call i16 @llvm.bswap.i16(i16 %a)
  ret i16 %1
}

InstCombiner::MatchBSwap is in llvm yet this works just fine with llvm-gcc (as well as other arches) so would you not expect the issue to reside in clang?

/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin/arm-apple-darwin10-llvm-gcc-4.2 -mcpu=cortex-m0 -mthumb -Os -S -emit-llvm revsh.c -o -
; ModuleID = 'revsh.c'
target datalayout = "e-p:32:32:32-i1:8:32-i8:8:32-i16:16:32-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:32:64-v128:32:128-a0:0:32-n32"
target triple = "thumbv6m-apple-darwin10"

define i32 @revsh(i16 zeroext %a) nounwind readnone optsize {
entry:
  %0 = tail call i16 @llvm.bswap.i16(i16 %a)
  %1 = sext i16 %0 to i32
  ret i32 %1
}

declare i16 @llvm.bswap.i16(i16) nounwind readnone

define zeroext i16 @bswapC(i16 zeroext %a) nounwind readnone optsize {
entry:
  %0 = tail call i16 @llvm.bswap.i16(i16 %a)
  ret i16 %0
}


> 
>> In the ARM architecture this can be accomplished with single instruction (REV16, or in the case I really care about REVSH)
>> 
>> Even with this patch LLVM generates two instructions: REV (which is 32 bit byte swap) and LSRS.
> 
> That's the default LegalizeTypes promotion of a bswap to a wider type;
> it should be possible to override in ARMISelLowering.cpp.

I'll take a look.

> 
>> I agree this should not be necessary if Clang be taught to recognize this itself, but I could not figure that out (yet). In the meantime this patch enables me to look into the LLVM half of generating the instructions.
>> 
>> I realize GCC only supports 32, and 64, but LLVM supports 16 as well and I did not see the harm in exposing it.
> 
> We prefer to expose as few builtins as possible, so people don't start
> using them and expect them to work in other compilers...
> 

Fair enough.


-James