[llvm-commits] [llvm] r123294 - in /llvm/trunk: lib/MC/ELFObjectWriter.cpp lib/Target/ARM/ARMAsmBackend.cpp lib/Target/ARM/ARMAsmPrinter.cpp lib/Target/ARM/ARMFixupKinds.h lib/Target/ARM/ARMMCCodeEmitter.cpp test/MC/ARM/elf-movt.s
Jim Grosbach
grosbach at apple.com
Wed Jan 12 13:56:57 PST 2011
Observations below.
On Jan 11, 2011, at 4:19 PM, Jason W Kim wrote:
> Author: jasonwkim
> Date: Tue Jan 11 18:19:25 2011
> New Revision: 123294
>
> URL: http://llvm.org/viewvc/llvm-project?rev=123294&view=rev
> Log:
> 1. Support ELF pcrel relocations for movw/movt:
> R_ARM_MOVT_PREL and R_ARM_MOVW_PREL_NC.
> 2. Fix minor bug in ARMAsmPrinter - treat bitfield flag as a bitfield, not an enum.
> 3. Add support for 3 new elf section types (no-ops)
>
>
>
> Modified:
> llvm/trunk/lib/MC/ELFObjectWriter.cpp
> llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp
> llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp
> llvm/trunk/lib/Target/ARM/ARMFixupKinds.h
> llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp
> llvm/trunk/test/MC/ARM/elf-movt.s
>
> Modified: llvm/trunk/lib/MC/ELFObjectWriter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/ELFObjectWriter.cpp?rev=123294&r1=123293&r2=123294&view=diff
> ==============================================================================
> --- llvm/trunk/lib/MC/ELFObjectWriter.cpp (original)
> +++ llvm/trunk/lib/MC/ELFObjectWriter.cpp Tue Jan 11 18:19:25 2011
> @@ -1268,6 +1268,9 @@
> case ELF::SHT_NOTE:
> case ELF::SHT_NULL:
> case ELF::SHT_ARM_ATTRIBUTES:
> + case ELF::SHT_INIT_ARRAY:
> + case ELF::SHT_FINI_ARRAY:
> + case ELF::SHT_PREINIT_ARRAY:
> // Nothing to do.
> break;
>
> @@ -1490,6 +1493,13 @@
> default:
> Type = ELF::R_ARM_CALL; break;
> } break;
> + case ARM::fixup_arm_movt_hi16:
> + case ARM::fixup_arm_movt_hi16_pcrel:
> + Type = ELF::R_ARM_MOVT_PREL; break;
> + case ARM::fixup_arm_movw_lo16:
> + case ARM::fixup_arm_movw_lo16_pcrel:
> + Type = ELF::R_ARM_MOVW_PREL_NC; break;
> +
Should the pc-rel and non-pc-rel fixups really both map to the same relocation? That doesn't seem right. If they're the same, they shouldn't need distinct fixups. If they're different, they should map differently.
The "break" statement should be on the following line, not the same line as the assignment. As a style thing, keeping the break on the same line is fine when the whole thing (including the case label) is on the same line for simple stuff. Once
there's already multiple lines, please separate them. The code above this gets very confusing due to this with nested switch statements and multiple breaks and such.
> }
> } else {
> switch ((unsigned)Fixup.getKind()) {
>
> Modified: llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp?rev=123294&r1=123293&r2=123294&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp Tue Jan 11 18:19:25 2011
> @@ -78,6 +78,8 @@
> { "fixup_arm_thumb_bcc", 1, 8, MCFixupKindInfo::FKF_IsPCRel },
> { "fixup_arm_movt_hi16", 0, 16, 0 },
> { "fixup_arm_movw_lo16", 0, 16, 0 },
> +{ "fixup_arm_movt_hi16_pcrel", 0, 16, MCFixupKindInfo::FKF_IsPCRel },
> +{ "fixup_arm_movw_lo16_pcrel", 0, 16, MCFixupKindInfo::FKF_IsPCRel },
> };
>
> if (Kind < FirstTargetFixupKind)
> @@ -156,7 +158,9 @@
> case FK_Data_4:
> return Value;
> case ARM::fixup_arm_movt_hi16:
> - case ARM::fixup_arm_movw_lo16: {
> + case ARM::fixup_arm_movw_lo16:
> + case ARM::fixup_arm_movt_hi16_pcrel:
> + case ARM::fixup_arm_movw_lo16_pcrel: {
> unsigned Hi4 = (Value & 0xF000) >> 12;
> unsigned Lo12 = Value & 0x0FFF;
> // inst{19-16} = Hi4;
>
> Modified: llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp?rev=123294&r1=123293&r2=123294&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp Tue Jan 11 18:19:25 2011
> @@ -189,10 +189,10 @@
> int64_t Imm = MO.getImm();
> O << '#';
> if ((Modifier && strcmp(Modifier, "lo16") == 0) ||
> - (TF == ARMII::MO_LO16))
> + (TF & ARMII::MO_LO16))
> O << ":lower16:";
> else if ((Modifier && strcmp(Modifier, "hi16") == 0) ||
> - (TF == ARMII::MO_HI16))
> + (TF & ARMII::MO_HI16))
As previously mentioned, you can just nuke the "Modifier" string compare. It's obsolete.
The value-kinds look like masks, but they're not consistently used that way, unfortunately. If you want to change them to work additively instead of a one-variant-kind-per-symbolref, that's great and I completely agree it's worth doing, but it really should be a separate patch. Until then, this isn't correct. Please revert this bit.
> O << ":upper16:";
> O << Imm;
> break;
>
> Modified: llvm/trunk/lib/Target/ARM/ARMFixupKinds.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMFixupKinds.h?rev=123294&r1=123293&r2=123294&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMFixupKinds.h (original)
> +++ llvm/trunk/lib/Target/ARM/ARMFixupKinds.h Tue Jan 11 18:19:25 2011
> @@ -74,6 +74,11 @@
> fixup_arm_movt_hi16, // :upper16:
> fixup_arm_movw_lo16, // :lower16:
>
> + // It is possible to create an "immediate" that happens to be pcrel.
I don't follow. Can you add an example?
> + // Needed to support ELF::R_ARM_MOVT_PREL and ELF::R_ARM_MOVW_PREL_NC
> + fixup_arm_movt_hi16_pcrel, // :upper16:
> + fixup_arm_movw_lo16_pcrel, // :lower16:
> +
> // Marker
> LastTargetFixupKind,
> NumTargetFixupKinds = LastTargetFixupKind - FirstTargetFixupKind
>
> Modified: llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp?rev=123294&r1=123293&r2=123294&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp Tue Jan 11 18:19:25 2011
> @@ -626,6 +626,32 @@
> return Binary;
> }
>
> +// FIXME: This routine needs to handle more MCExpr types
> +static const MCSymbolRefExpr *FindLHSymExpr(const MCExpr *E) {
> + // recurse left child until finding a MCSymbolRefExpr
> + switch (E->getKind()) {
> + case MCExpr::SymbolRef:
> + return cast<MCSymbolRefExpr>(E);
> + case MCExpr::Binary:
> + return FindLHSymExpr(cast<MCBinaryExpr>(E)->getLHS());
> + default:
> + return NULL;
> + }
> +}
> +
> +// FIXME: This routine assumes that a binary
> +// expression will always result in a PCRel expression
> +// In reality, its only true if one or more subexpressions
> +// is itself a PCRel (i.e. "." in asm or some other pcrel construct)
> +// but this is good enough for now.
This comment makes me very, very nervous. LLVM isn't always compiling ARM code as PIC. Can you help me understand why this is indeed good enough for now?
> +static bool EvaluateAsPCRel(const MCExpr *Expr) {
> + switch (Expr->getKind()) {
> + case MCExpr::SymbolRef: return false;
> + case MCExpr::Binary: return true;
> + default: assert(0 && "Unexpected expression type");
> + }
> +}
> +
> uint32_t ARMMCCodeEmitter::
> getMovtImmOpValue(const MCInst &MI, unsigned OpIdx,
> SmallVectorImpl<MCFixup> &Fixups) const {
> @@ -635,18 +661,27 @@
> if (MO.isImm()) {
> return static_cast<unsigned>(MO.getImm());
> } else if (const MCSymbolRefExpr *Expr =
> - dyn_cast<MCSymbolRefExpr>(MO.getExpr())) {
> + FindLHSymExpr(MO.getExpr())) {
> + // FIXME: :lower16: and :upper16: should be applicable to
> + // to whole expression, not just symbolrefs
> + // Until that change takes place, this hack is required to
> + // generate working code.
> + const MCExpr *OrigExpr = MO.getExpr();
> MCFixupKind Kind;
> switch (Expr->getKind()) {
> default: assert(0 && "Unsupported ARMFixup");
> case MCSymbolRefExpr::VK_ARM_HI16:
> Kind = MCFixupKind(ARM::fixup_arm_movt_hi16);
> + if (EvaluateAsPCRel(OrigExpr))
> + Kind = MCFixupKind(ARM::fixup_arm_movt_hi16_pcrel);
> break;
> case MCSymbolRefExpr::VK_ARM_LO16:
> Kind = MCFixupKind(ARM::fixup_arm_movw_lo16);
> + if (EvaluateAsPCRel(OrigExpr))
> + Kind = MCFixupKind(ARM::fixup_arm_movw_lo16_pcrel);
> break;
> }
> - Fixups.push_back(MCFixup::Create(0, Expr, Kind));
> + Fixups.push_back(MCFixup::Create(0, OrigExpr, Kind));
> return 0;
> };
> llvm_unreachable("Unsupported MCExpr type in MCOperand!");
>
> Modified: llvm/trunk/test/MC/ARM/elf-movt.s
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/MC/ARM/elf-movt.s?rev=123294&r1=123293&r2=123294&view=diff
> ==============================================================================
> --- llvm/trunk/test/MC/ARM/elf-movt.s (original)
> +++ llvm/trunk/test/MC/ARM/elf-movt.s Tue Jan 11 18:19:25 2011
> @@ -1,4 +1,6 @@
> @ RUN: llvm-mc %s -triple=armv7-linux-gnueabi | FileCheck -check-prefix=ASM %s
> +@ RUN: llvm-mc %s -triple=armv7-linux-gnueabi -filetype=obj -o - | \
> +@ RUN: elf-dump --dump-section-data | FileCheck -check-prefix=OBJ %s
> .syntax unified
> .text
> .globl barf
> @@ -12,3 +14,26 @@
> @ ASM: movw r0, :lower16:GOT-(.LPC0_2+8)
> @ ASM-NEXT: movt r0, :upper16:GOT-(.LPC0_2+16)
>
> +@@ make sure that the text section fixups are sane too
> +@ OBJ: '.text'
> +@ OBJ-NEXT: 'sh_type', 0x00000001
> +@ OBJ-NEXT: 'sh_flags', 0x00000006
> +@ OBJ-NEXT: 'sh_addr', 0x00000000
> +@ OBJ-NEXT: 'sh_offset', 0x00000034
> +@ OBJ-NEXT: 'sh_size', 0x00000008
> +@ OBJ-NEXT: 'sh_link', 0x00000000
> +@ OBJ-NEXT: 'sh_info', 0x00000000
> +@ OBJ-NEXT: 'sh_addralign', 0x00000004
> +@ OBJ-NEXT: 'sh_entsize', 0x00000000
> +@ OBJ-NEXT: '_section_data', 'f00f0fe3 ec0f4fe3'
> +
> +@ OBJ: Relocation 0x00000000
> +@ OBJ-NEXT: 'r_offset', 0x00000000
> +@ OBJ-NEXT: 'r_sym'
> +@ OBJ-NEXT: 'r_type', 0x0000002d
> +
> +@ OBJ: Relocation 0x00000001
> +@ OBJ-NEXT: 'r_offset', 0x00000004
> +@ OBJ-NEXT: 'r_sym'
> +@ OBJ-NEXT: 'r_type', 0x0000002e
> +
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list