[PATCH] ARM: For thumb fixups store halfwords high first and low second

[Jim Grosbach]

Purely a style comment; I haven’t looked at the technical content of the patch.

Function names begin w/ lower case. Check the llvm coding guidelines for details. For example, JoinHalfWords() should be joinHalfWords().

-Jim

On Apr 17, 2014, at 6:34 PM, Saleem Abdulrasool <compnerd at compnerd.org> wrote:

> 
>  Thanks for fixing this.
> 
> 
> ================
> Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:317
> @@ +316,3 @@
> +  }
> +  else
> +    return Value;
> ----------------
> The else is unnecessary.
> 
> ================
> Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:328
> @@ +327,3 @@
> +    Value |= FirstHalf;
> +  }
> +  else {
> ----------------
> The else should be coddled:
> 
>    } else {
> 
> ================
> Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:335
> @@ +334,3 @@
> +  return Value;
> +}
> +
> ----------------
> This is purely a stylistic nit, and the choice is yours to make, but I find this to be just as readable, but more concise:
> 
>    static uint32_t JoinHalfWords(uint32_t First, uint32_t Second,
>                                  bool IsLittleEndian) {
>      if (IsLittleEndian)
>        return (Second & 0xffff << 16) | (First & 0xffff << 0);
>      return (First & 0xffff << 16) | (Second & 0xffff << 0);
>    }
> 
> ================
> Comment at: lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp:326
> @@ +325,3 @@
> +  if (IsLittleEndian) {
> +    Value |= SecondHalf << 16;
> +    Value |= FirstHalf;
> ----------------
> Can you please mask the lower halves of the words since you are passing in uint32_t rather than uint16_t?
> 
> ================
> Comment at: test/MC/ARM/thumb2be-b.w-encoding.s:1
> @@ +1,2 @@
> +// RUN: llvm-mc -triple=thumbebv7-none-linux-gnueabi -mcpu=cortex-a8 -filetype=obj < %s | llvm-objdump -arch=thumbeb -s - | FileCheck %s
> +  .syntax unified
> ----------------
> Can you change the test to test both little and big endian?  -check-prefix should allow you to check both variants in the same file.
> 
> 
> http://reviews.llvm.org/D3380
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits