[LLVMdev] ARMCodeEmitter.cpp JIT support very broken (2.9 and svn)

[Craig Smith]

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.