Page MenuHomePhabricator

ARM: For thumb fixups store halfwords high first and low second
ClosedPublic

Authored by cpirker on Apr 15 2014, 9:54 AM.

Details

Reviewers
compnerd
Summary

Hi All,

This patch will work for little and big endian.
Please review.

Thanks,
Christian

Diff Detail

Event Timeline

These changes look reasonable to me and do improve things on our ABI test suite.

But it should include one or more regression tests in llvm/test/MC/ARM along the lines of:

thumb2be-b.w-encoding.s

@ RUN: llvm-mc -triple=thumbebv7-none-linux-gnueabi -mcpu=cortex-a8 -filetype=obj < %s | llvm-objdump -arch=thumbeb -s - | FileCheck %s
  .syntax unified
  .code	16
  .thumb_func
foo:
  b.w   bar

@ CHECK: Contents of section .text:
@ CHECK-NEXT: 0000 f7ffbffe

I'd rather use 'llvm-objdump -arch=thumbeb -disassemble -' instead of '-s' but llvm-objdump seems broken for -arch=thumbeb (which would be good to fix too, separately).

compnerd added inline comments.Apr 16 2014, 10:17 AM
lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
309

Why not call it SwapHalfWords? It isnt tied to a MCFixup.

358

No space after the function name. Likewise elsewhere.

484

I think an auxiliary function like:

uint32_t JoinHalfWords(uint16_t First, uint16_t Second, bool IsLittleEndian);

might be nice in this case since we could reuse it in the cases where we need.

Also, the else should be coddled:

} else {
cpirker updated this revision to Unknown Object (????).Apr 17 2014, 8:39 AM

Hi All,

I updated the functions name SwapHalfWords and added the function JoinHalfWords.
I also added a test file.

Thanks,
Christian

Thanks for fixing this.

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
317

The else is unnecessary.

326

Can you please mask the lower halves of the words since you are passing in uint32_t rather than uint16_t?

328

The else should be coddled:

} else {
335

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);
}
test/MC/ARM/thumb2be-b.w-encoding.s
2

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.

Purely a style comment; I haven

cpirker updated this revision to Diff 8801.Apr 24 2014, 5:44 AM

Hi,

I updated the source code as noted in the comments.
I added also a little endian test in the test file.
Please review.

Thanks,
Christian

cpirker updated this revision to Diff 8803.Apr 24 2014, 6:25 AM

Hi,

I made a mistake in my diff file.
There was a ')' missing in the function "joinHalfWords"

Thanks,
Christian

compnerd accepted this revision.Apr 24 2014, 7:36 AM
compnerd added a reviewer: compnerd.

Thanks for addressing all the feedback!

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
311

I wouldve put this as a function comment.

This revision is now accepted and ready to land.Apr 24 2014, 7:36 AM
cpirker closed this revision.May 6 2014, 3:14 AM

I committed this patch as r208076.