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,

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

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.

Hi Craig,

The problem with this is that the ARM JIT was never gotten to "supported" status at any point, so regressions were not monitored. The code path is essentially dead, at the moment, with noone willing to invest time in flogging a dead horse as it'll all have to be rewritten when MC lands properly and someone has the time/inclination to architect it.

I understand your frustration at it being allowed to code rot, but if it was never complete with proper tests and noone cares enough, that is naturally going to happen.

Cheers,

James

Is MC JIT support expected in 3.0, and if not, what does the timeline look like?

Would I be better off trying to get the supported but incomplete MC JITter working than spending effort preserving the dead branch?

Thanks
Craig

Is MC JIT support expected in 3.0, and if not, what does the timeline look like?

Would I be better off trying to get the supported but incomplete MC JITter working than spending effort preserving the dead branch?

  Currently, our project relies on LLVM 2.8 ARM JIT. I would like to know
if we can move to MC JIT seamlessly.

Regards,
chenwj

Hi,

3.0 has been branched already AFAIK - there is no way MC JIT for ARM will be in the 3.0 release.

Most of the effort on the ARM front is going into getting the MC production quality, which can also be seen as a prerequisite for MC JIT.

Cheers,

James

Not at all. :slight_smile:

-eric

Apologies, thought I saw talk a while back about the branch on the list! :confused:

James

I think you did, but the person was in error. :slight_smile: 3.0 won't branch until we get close to the release date, which hasn't been set yet.

-bw