[llvm-commits] [llvm] r121408 - in /llvm/trunk: include/llvm/MC/MCCodeEmitter.h lib/MC/MCAssembler.cpp lib/Target/ARM/ARMAsmBackend.cpp lib/Target/ARM/ARMMCCodeEmitter.cpp

[Daniel Dunbar]

Hi Owen,

On Thu, Dec 9, 2010 at 12:27 PM, Owen Anderson <resistor at mac.com> wrote:
> Author: resistor
> Date: Thu Dec  9 14:27:52 2010
> New Revision: 121408
>
> URL: http://llvm.org/viewvc/llvm-project?rev=121408&view=rev
> Log:
> Fix an issue in some Thumb fixups, where the effective PC address needs to be 4-byte aligned when calculating
> the offset.  Add a new fixup flag to represent this, and use it for the one fixups that I have a testcase for needing
> this.  It's quite likely that the other Thumb fixups will need this too, and to have their fixup encoding logic
> adjusted accordingly.

Ok; but I have a number of concerns here...

First and most critically, please factor any MC infrastructure changes
into separate commits. I don't really care how the individual targets
work (we can fix it incrementally), but I do care about trying to get
the core MC design "right".

Subsequent comments inline...

> Modified:
>    llvm/trunk/include/llvm/MC/MCCodeEmitter.h
>    llvm/trunk/lib/MC/MCAssembler.cpp
>    llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp
>    llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp
>
> Modified: llvm/trunk/include/llvm/MC/MCCodeEmitter.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCCodeEmitter.h?rev=121408&r1=121407&r2=121408&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/MC/MCCodeEmitter.h (original)
> +++ llvm/trunk/include/llvm/MC/MCCodeEmitter.h Thu Dec  9 14:27:52 2010
> @@ -25,7 +25,10 @@
>   enum FixupKindFlags {
>     /// Is this fixup kind PCrelative? This is used by the assembler backend to
>     /// evaluate fixup values in a target independent manner when possible.
> -    FKF_IsPCRel = (1 << 0)
> +    FKF_IsPCRel = (1 << 0),
> +
> +    // Should this fixup kind force a 4-byte aligned effective PC value?
> +    FKF_IsAligned = (1 << 1)

Please use doxyments where appropriate.

I find the name is underspecified:
  1. IsAligned doesn't include any mention of the magic "4" number.
  2. Is there any chance any non ARM platform will use this?
  3. Calling this "aligned" is a bit of a misnomer. Aligned in most
places in the compiler means "aligned up", and this means "aligned
down".
Can we call it something like "IsAlignedDownToWord", and make the
semantics match that? Or IsAlignedDownTo32Bits?

>   };
>
>   /// A target specific name for the fixup kind. The names will be unique for
>
> Modified: llvm/trunk/lib/MC/MCAssembler.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCAssembler.cpp?rev=121408&r1=121407&r2=121408&view=diff
> ==============================================================================
> --- llvm/trunk/lib/MC/MCAssembler.cpp (original)
> +++ llvm/trunk/lib/MC/MCAssembler.cpp Thu Dec  9 14:27:52 2010
> @@ -253,8 +253,15 @@
>   if (IsResolved)
>     IsResolved = Writer.IsFixupFullyResolved(*this, Target, IsPCRel, DF);
>
> -  if (IsPCRel)
> -    Value -= Layout.getFragmentOffset(DF) + Fixup.getOffset();
> +  if (IsPCRel) {
> +    bool ShouldAlignPC = Emitter.getFixupKindInfo(
> +                        Fixup.getKind()).Flags & MCFixupKindInfo::FKF_IsAligned;
> +    // PC should be aligned to a 4-byte value.
> +    if (ShouldAlignPC)
> +      Value -= Layout.getFragmentOffset(DF) + (Fixup.getOffset() & ~0x3);
> +    else
> +      Value -= Layout.getFragmentOffset(DF) + Fixup.getOffset();
> +  }

I would write this as:
--
uint32_t Offset = Fixup.getOfset()

if (ShouldAlignPC)
  Offset = Offset & ~0x3;

Value -= Layout.getFragmentOffset(DF) + Offset;
--
Which makes more sense to me (treating these as flags).

I'd also love to see an assert somewhere that this flag can't be set
without IsPCRel.

Thanks!
 - Daniel

>
>   return IsResolved;
>  }
>
> Modified: llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp?rev=121408&r1=121407&r2=121408&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMAsmBackend.cpp Thu Dec  9 14:27:52 2010
> @@ -100,10 +100,10 @@
>   }
>   case ARM::fixup_arm_ldst_pcrel_12:
>     // ARM PC-relative values are offset by 8.
> -    Value -= 6;
> +    Value -= 4;
>   case ARM::fixup_t2_ldst_pcrel_12: {
>     // Offset by 4, adjusted by two due to the half-word ordering of thumb.
> -    Value -= 2;
> +    Value -= 4;
>     bool isAdd = true;
>     if ((int64_t)Value < 0) {
>       Value = -Value;
>
> Modified: llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp?rev=121408&r1=121407&r2=121408&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMMCCodeEmitter.cpp Thu Dec  9 14:27:52 2010
> @@ -47,7 +47,8 @@
>     const static MCFixupKindInfo Infos[] = {
>       // name                       off   bits  flags
>       { "fixup_arm_ldst_pcrel_12",  1,    24,   MCFixupKindInfo::FKF_IsPCRel },
> -      { "fixup_t2_ldst_pcrel_12",   0,    32,   MCFixupKindInfo::FKF_IsPCRel },
> +      { "fixup_t2_ldst_pcrel_12",   0,    32,   MCFixupKindInfo::FKF_IsPCRel |
> +                                                MCFixupKindInfo::FKF_IsAligned},
>       { "fixup_arm_pcrel_10",       1,    24,   MCFixupKindInfo::FKF_IsPCRel },
>       { "fixup_t2_pcrel_10",        0,    32,   MCFixupKindInfo::FKF_IsPCRel },
>       { "fixup_arm_adr_pcrel_12",   1,    24,   MCFixupKindInfo::FKF_IsPCRel },
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>