Page MenuHomePhabricator

[PPCInstPrinter] Change printBranchOperand(calltarget) to print the target address in hexadecimal form
ClosedPublic

Authored by MaskRay on Mar 23 2020, 12:19 AM.

Details

Summary
// llvm-objdump -d output (before)
0: bl .-4
4: bl .+0
8: bl .+4

// llvm-objdump -d output (after) ; GNU objdump -d
0: bl 0xfffffffc / bl 0xfffffffffffffffc
4: bl 0x4
8: bl 0xc

Many Operand's are not annotated as OPERAND_PCREL. They are not affected (e.g. b .+67108860). I plan to fix them in future patches.

Modified test/tools/llvm-objdump/ELF/PowerPC/branch-offset.s to test
address space wraparound for powerpc32 and powerpc64.

Diff Detail

Event Timeline

MaskRay created this revision.Mar 23 2020, 12:19 AM
jhenderson added inline comments.Mar 24 2020, 2:26 AM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
421–422

Not being a PPC expert, what is the << 2 all about?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.h
68

Don't miss this clang-format request!

This seems perfectly fine with me - and more consistent with GNU binutils.
However, I think the AIX guys should also have a look as I am not sure what the system assembler's capabilities are on AIX. I'll add a couple of AIX reviewers.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
421–422

The branches on PPC must branch to word-aligned (4-byte word) addresses. So the branch address is the encoded value shifted two bits to the left (plus PC for the PC-relative ones).

jasonliu added a comment.EditedMar 24 2020, 8:31 AM

@nemanjai
AIX system assembler does not seem to like this new form.

Assembler:
test.s: line 46: 1252-086 The target of the branch instruction
        must be a relocatable or external expression.

@nemanjai
AIX system assembler does not seem to like this new form.

Assembler:
test.s: line 46: 1252-086 The target of the branch instruction
        must be a relocatable or external expression.

llvm-objdump -d output may not be assembled back. That said, I think most users like the PC relative immediate to be displayed as the target address. If AIX users don't agree, I can special case it.

To maintain/correct the AIX output for the tools there seems to be a prerequisite fix to llvm::object::ObjectFile::makeTriple.

MaskRay updated this revision to Diff 252360.Mar 24 2020, 10:03 AM

Force relative form bl .+ on AIX.

sfertile added inline comments.Mar 24 2020, 10:14 AM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
438

Do we have a way to print the hex value without a leading '0x'? If we are making the effort to match binutils objump, why wouldn't we match exactly for this case?

MaskRay marked an inline comment as done.Mar 24 2020, 11:15 AM
MaskRay added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
438

Don't use formatHex then we are done. However, I actually think an explicit 0x is better.

A GNU objdump -d result (x86_64) cannot be re-assembled back. The omission of 0x is one of the reasons.

@nemanjai
AIX system assembler does not seem to like this new form.

Assembler:
test.s: line 46: 1252-086 The target of the branch instruction
        must be a relocatable or external expression.

llvm-objdump -d output may not be assembled back. That said, I think most users like the PC relative immediate to be displayed as the target address. If AIX users don't agree, I can special case it.

Sorry for the churn but I think your initial approach is correct. objdump on AIX also prints the target address as hex and like you say, the output isn't meant to be assembled back. Any cases where we the output is asm are unaltered right?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
438

I don't feel too strongly either way but I though the tools that are replicating binutils tools are meant to be exact drop-in replacements outside of replicating bugs. If this is one of many things that precludes objdumps output from being disassembled then we should prioritize compatibility, otherwise I have no reservations about keeping the leading `0x'.

MaskRay updated this revision to Diff 252435.Mar 24 2020, 3:02 PM
MaskRay edited the summary of this revision. (Show Details)

The new form actually matches GNU objdump -d output.

In GNU objdump, on x86-64, 0x is omitted. On powerpc, 0x is kept...

MaskRay marked 2 inline comments as done.Mar 24 2020, 3:03 PM
MaskRay added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
438

Sorry, I was mistaken. GNU objdump -d on powerpc actually keeps the leading 0x. So we are actually consistent with GNU objdump.

sfertile accepted this revision as: sfertile.Mar 25 2020, 6:37 AM

LGTM.

This revision is now accepted and ready to land.Mar 25 2020, 6:37 AM
jhenderson accepted this revision.Mar 26 2020, 1:44 AM

LGTM too.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
421–422

Thanks!

This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.