[PATCH] D37030: Fix ARMv4 support

[Joerg Sonnenberger via Phabricator via llvm-commits]

joerg added inline comments.


================
Comment at: lib/Target/ARM/ARMAsmPrinter.cpp:1275
 
+    assert(Subtarget->hasV4TOps());
     EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::BX)
----------------
rengolin wrote:
> fhahn wrote:
> > Maybe this assert would be clearer if the assert contained something like && "BX instructions are only available in ARMv4t and later". Applies to all added asserts in this patch
> Well, both `BX_CALL` and `BX_RET` have `Requires<[IsARM, HasV4T]>`, which `ARMv4` (in `ARM.td`) doesn't have.
> 
> So, either this `BX_CALL` is being generated manually (and incorrectly), or there's something wrong with the ISel.
> 
> I think it's safe to assume that, if a target has `BX_CALL`, it also has `BX_RET`, so the assert here should be redundant.
> 
> Your princess might be in another castle. :)
> 
> PS: It's possible that the introduction of `getReturnOpcode()` has fixed that already.
Yes, it is pretty obvious from the instructions why the assert exists. The main motivation for the asserts is to ensure that all places where we create them by hand also have the feature around. Given that we missed a couple of them, I prefer to be safe here.


================
Comment at: lib/Target/ARM/ARMExpandPseudoInsts.cpp:1028
+          STI->isThumb() ? ARM::tTAILJMPr
+                         : (STI->hasV4TOps() ? ARM::TAILJMPr : ARM::TAILJMPr4);
         BuildMI(MBB, MBBI, dl,
----------------
olista01 wrote:
> Could you add a test covering the ARM::TAILJMPr4 case?
armv4.ll tests the tail call.


================
Comment at: lib/Target/ARM/ARMLoadStoreOptimizer.cpp:1912
     if (Use.isKill()) {
+      assert(STI->hasV4TOps());
       BuildMI(MBB, MBBI, MBBI->getDebugLoc(), TII->get(ARM::tBX))
----------------
fhahn wrote:
> I'm not sure if this assertion is really needed here. If the block does not already contain a BX instruction, we exit early.
I don't think it hurts either, though. It makes it easier to search for potentially missing places.


Repository:
  rL LLVM

https://reviews.llvm.org/D37030