[llvm-commits] [patch] ARM/MC/ELF add new stub for movt/movw in ARMFixupKinds

[Jason Kim]

On Wed, Nov 17, 2010 at 8:45 AM, Jason Kim <jasonwkim at google.com> wrote:
> -llvmdev
> +llvmcommits
>
> On Tue, Nov 16, 2010 at 4:27 PM, Jim Grosbach <grosbach at apple.com> wrote:
>>
>> On Nov 16, 2010, at 4:01 PM, Jason Kim wrote:
>>
>> +llvmdev
>> -llvmcommits
>>
>> On Fri, Nov 12, 2010 at 8:03 AM, Jim Grosbach <grosbach at apple.com> wrote:
>>
>> Sorta. getBinaryCodeForInst() is auto-generated by tablegen, so shouldn't be
>> modified directly. The target can register hooks for instruction operands
>> for any special encoding needs, including registering fixups, using the
>> EncoderMethod string. For an example, have a look at the LDRi12 instruction
>> and how it registers a fixup for the addrmode_imm12 operand when it needs
>> one.
>>
>> Hi Jim,. follow up question for ya:
>>
>> The current movt/movw pair (as defined in ARMInstrInfo.td)  does not
>> use EncoderMethod string to declare  a special case handler.
>>
>> There's two parts to how this works. First, is the handling of the movt/movw
>> pair for instruction selection, and then the handling of the upper/lower
>> operand flags for those instructions.
>> MOVi32imm is expanded as a pseudo instruction in ARMExpandPseudos pass, and
>> any other patterns that do similar things should also be expanded there. For
>> a few special cases where we need the post-RA scheduler to handle the
>> instruction sequences as an atomic unit (PICLDR, PICADD, the
>> eh.sjlj.setjmp/longjmp patterns, for example), we expand them even later,
>> during MC lowering in ARMAsmPrinter.cpp. As a result, the movt and movw
>> instructions should make it into the code emitter as separate entries, not
>> as a composite pair.
>> For operands that aren't compile-time constants, the operand flag gets set
>> in the expansion to the MOVT/MOVW instructions:
>>       if (MO.isImm()) {
>>         unsigned Imm = MO.getImm();
>>         unsigned Lo16 = Imm & 0xffff;
>>         unsigned Hi16 = (Imm >> 16) & 0xffff;
>>         LO16 = LO16.addImm(Lo16);
>>         HI16 = HI16.addImm(Hi16);
>>       } else {
>>         const GlobalValue *GV = MO.getGlobal();
>>         unsigned TF = MO.getTargetFlags();
>>         LO16 = LO16.addGlobalAddress(GV, MO.getOffset(), TF |
>> ARMII::MO_LO16);
>>         HI16 = HI16.addGlobalAddress(GV, MO.getOffset(), TF |
>> ARMII::MO_HI16);
>>       }
>> That's then marked with VK_ARM_LO16/VK_ARM_HI16 by ARMMCInstLower.cpp when
>> it's looking at symbol reference operands.
>>
>> At the current time, for the assembly printing,
>> MCAsmStreamer::EmitInstruction(const MCInst &Inst) calls out to
>>  MCExpr::print(raw_ostream &OS)
>>   which then calls out to MCSymbolRefExpr::getVariantKindName() to
>> print the magic :lower16: and :upper16: asm tags for .s emission
>> Currently, movt/movw emission works correctly in .s, but not in .o emission
>>
>> This lead me to believe that the correct place to put the code to handle
>> MCSymbolRefExpr::VK_ARM_(HI||LO)16 for the .o path was to place a case
>> in getMachineOpValue() (i.e. not
>> ARMMCCodeEmitter::getBinaryCodeForInstr like I mistakenly wrote in my
>> prior email.)
>>
>> Are you implying that the movt/movw instruction definition in the .td
>> files need to be fixed up instead to declare a new special case for .o
>> emission via the EncoderMethod string, for the .o emission of
>> movt/movw to be considered "correct"?
>>
>> A custom encoder may be the best way to differentiate the kinds of fixups
>> we'll need for these operands (e.g., using LO16 may generate different
>> fixups depending on which instruction it's used for, which implies different
>> operand node types, and thus different encoder methods). In any case, the
>> encoder, be it in getMachineOpValue() or a specialized method, will need to
>> differentiate between an immediate operand and a symbol reference expression
>> operand. In the latter case, it'll create a fixup for the lo/hi variantkind
>> if one is present.
>
> Got ya. Is this more in tune with what you had in mind? (applies cleanly to
> -r119149)

Yikes. As sent, patch is missing two enum definitions
The update is here.
Sorry for the noise (again ):
-jason

>
> Thanks!
>
>> -Jim
>>
>> On Nov 11, 2010, at 7:06 PM, Jason Kim wrote:
>>
>> Is getBinaryCodeForInst the best place to place the case for
>>
>> supporting movt/movw fixup emission?
>>
>> The call stack seems to be:
>>
>> #0 ARMMCCodeEmitter::getBinaryCodeForInstr
>>
>> #1 ARMMCCodeEmitter::EncodeInstruction
>>
>> #2 MCELFStreamer::EmitInstToData
>>
>> #3 MCObjectStreamer::EmitInstruction
>>
>> #4 ARMAsmPrinter::EmitInstruction
>>
>> <arm-mc-elf-s08.patch>_______________________________________________
>>
>> llvm-commits mailing list
>>
>> llvm-commits at cs.uiuc.edu
>>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm-mc-elf-s09b-movt.patch
Type: text/x-patch
Size: 6210 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101117/cd37ac46/attachment.bin>