llvm dev - Aug 2011 - [LLVMdev] ARMCodeEmitter.cpp JIT support very broken (2.9 and svn)

I recently tried to update from LLVM 2.8 and 2.9 and ran into several bad issues
with JIT support on ARM.
I ran into several distinct issues so far, and there are probably others.  
(None of these problems appear to be fixed in the current svn head either as far
as I can tell.)

1) VFP/Neon instructions don't encode correctly at al, because the encoding
methods generated by tablegen for them clobber the constructed binary value when
they try to implement 'PostEncoderMethod' support , for example, from
ARMGenCodeEmitter.inc:

    case ARM::VLDRD:
    case ARM::VSTRD: {
      // op: p
      op = getMachineOpValue(MI, MI.getOperand(3));
      Value |= (op & 15U) << 28;
     // etc ...
      Value = VFPThumb2PostEncoder(MI, Value);         // <--- overwrites
Value!
      break;
    }

The bug here is that in utils/TableGen/CodeEmitterGen.cpp, line 196:
	Case += "      Value = " + PostEmitter + "(MI, Value);\n";
should be
	Case += "      Value |= " + PostEmitter + "(MI, Value);\n";

This looks like it would affect all targets, except apparently only ARM uses
this feature.

2) ARM BR_JTm and BR_JTadd do not emit because they were changed to
PseudoInstructions but the ARMCodeEmitter wasn't updated to compensate.  
emitPseudoInstruction() asserts (llvm_unreachable).

3) FCONSTS/FCONSTD also assert similarly.   emitMiscInstruciton which used to
support these instructions was removed in r116644.  If you try to add back a
case for them in the obvious way, getBinaryCodeForInstr() (which now ostensibly
should handle this and has a case for it) asserts constructing the instruction
because getMachineOpValue(MI, MI,getOperand(1)) doesn't handle a
MachineOperand of type FPImm.

I'm not sure what the right way to fix these last to issues is.

Is any regression testing done at all for JIT support on non-X86 platforms?  It
seems like a simple cross-compilation framework could be set up to allow such
targets to be sanity-tested without needing an actual CPU to run one.

Thanks

Craig,

On Aug 30, 2011, at 12:51 PM, Craig Smith wrote:
> I recently tried to update from LLVM 2.8 and 2.9 and ran into several bad
issues with JIT support on ARM.
> I ran into several distinct issues so far, and there are probably others.  
(None of these problems appear to be fixed in the current svn head either as far
as I can tell.)
The non-MC-based ARM JIT path is known not to work, and nobody is working on
fixing it.  The MC-based instruction encoder is rapidly maturing is generally
passable for static encoding, but the MCJIT is still in its infancy.
> 1) VFP/Neon instructions don't encode correctly at al, because the
encoding methods generated by tablegen for them clobber the constructed binary
value when they try to implement 'PostEncoderMethod' support , for
example, from ARMGenCodeEmitter.inc:
> 
>    case ARM::VLDRD:
>    case ARM::VSTRD: {
>      // op: p
>      op = getMachineOpValue(MI, MI.getOperand(3));
>      Value |= (op & 15U) << 28;
>     // etc ...
>      Value = VFPThumb2PostEncoder(MI, Value);         // <--- overwrites
Value!
>      break;
>    }
> 
> The bug here is that in utils/TableGen/CodeEmitterGen.cpp, line 196:
> 	Case += "      Value = " + PostEmitter + "(MI,
Value);\n";
> should be
> 	Case += "      Value |= " + PostEmitter + "(MI,
Value);\n";
> 
This is the intended behavior.  The some post-encoder hooks need to clear bits
as well as set them.  If you're seeing incorrect output from the
post-encoder hook, it's because the hook itself has a bug.
> This looks like it would affect all targets, except apparently only ARM
uses this feature.
> 
> 2) ARM BR_JTm and BR_JTadd do not emit because they were changed to
PseudoInstructions but the ARMCodeEmitter wasn't updated to compensate.  
emitPseudoInstruction() asserts (llvm_unreachable).
This is another symptom of the non-MC ARM JIT being unmaintained.  It is correct
for emitPseudoInstruction() to assert.  All pseudo instructions should be
expanded before they reach the encoder, and they are properly expanded in the
MC-based path.

--Owen

On Aug 30, 2011, at 3:12 PM, Owen Anderson wrote:
> The non-MC-based ARM JIT path is known not to work, and nobody is working
on fixing it.  The MC-based instruction encoder is rapidly maturing is generally
passable for static encoding, but the MCJIT is still in its infancy.
I was relying on this support in LLVM 2.8, and while it is definitely
incomplete, it does work if you don't depend on certain features.  I found
it worked adequately as long as you set the -thumb2,-t2xtpk attributes.

It would be nice if at least it didn't get further broken until the MC
Emitter was fully ready to take its place.   The first FIXME comment in
ARMCodeEmitter seems to indicate this is a goal.: "… They are placeholders
to allow this encoder to continue to function until the MC encoder is
sufficiently far along that this one can be eliminated entirely."
>> 1) VFP/Neon instructions don't encode correctly at al, because the
encoding methods generated by tablegen for them clobber the constructed binary
value when they try to implement 'PostEncoderMethod' support , for
example, from ARMGenCodeEmitter.inc:
>> 
>> 
>> The bug here is that in utils/TableGen/CodeEmitterGen.cpp, line 196:
>> 	Case += "      Value = " + PostEmitter + "(MI,
Value);\n";
>> should be
>> 	Case += "      Value |= " + PostEmitter + "(MI,
Value);\n";
>> 
> 
> This is the intended behavior.  The some post-encoder hooks need to clear
bits as well as set them.  If you're seeing incorrect output from the
post-encoder hook, it's because the hook itself has a bug.
Ah, ok.  So it looks like the stubs in ARMCodeEmitter.cpp such as 
unsigned VFPThumb2PostEncoder(const MachineInstr&MI, unsigned Val) const {
return 0; }
should instead be 
unsigned VFPThumb2PostEncoder(const MachineInstr&MI, unsigned Val) const {
return Val; }

>> This looks like it would affect all targets, except apparently only ARM
uses this feature.
>> 
>> 2) ARM BR_JTm and BR_JTadd do not emit because they were changed to
PseudoInstructions but the ARMCodeEmitter wasn't updated to compensate.  
emitPseudoInstruction() asserts (llvm_unreachable).
> 
> This is another symptom of the non-MC ARM JIT being unmaintained.  It is
correct for emitPseudoInstruction() to assert.  All pseudo instructions should
be expanded before they reach the encoder, and they are properly expanded in the
MC-based path.
But there are several PseudoInstruciton still around and handled here in the
non-MC based path.   Again, it'd be nice to not obsolete old code until
it's replacement is actually ready.