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

+llvmdev
-llvmcommits

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.

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"?
(If so, I have a few more followup^2 quesitions :slight_smile:

Thanks!

-jason

+llvmdev
-llvmcommits

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.

-Jim