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

Saleem Abdulrasool compnerd at compnerd.org
Thu Apr 17 18:34:19 PDT 2014


  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






More information about the llvm-commits mailing list