[PATCH] D39592: [ARM|GlobalISel] : Adding legalizer tests for Thumb

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: lib/Target/ARM/ARMLegalizerInfo.cpp:60
       setAction({Op, Ty}, WidenScalar);
-    if (ST.hasDivideInARMMode())
+    if (ST.hasDivideInARMMode() || ST.hasDivideInThumbMode())
       setAction({Op, s32}, Legal);
----------------
You should check if you're generating ARM or Thumb, since you don't want the Thumb hardware divide to influence ARM code generation and vice versa. So, something like (ST.isThumb() && ST.hasDivideInThumbMode()) || (!ST.isThumb() && ST.hasDivideInARMMode())


================
Comment at: test/CodeGen/ARM/GlobalISel/legalize-divmod.mir:53
+    ; SOFT-DAG: %r1 = COPY [[Y]]
+    ; SOFT-AEABI: BL $__aeabi_idiv, {{.*}}, implicit %r0, implicit %r1, implicit-def %r0
+    ; SOFT-AEABI: [[R:%[0-9]+]]:_(s32) = COPY %r0
----------------
javed.absar wrote:
> rovka wrote:
> > Hmm, this shouldn't be a BL for Thumb. You'll have to update ARMCallLowering to handle Thumb before we can test this properly.
> Would you recommend then that I hold on to this patch till I have sorted out CallLowering for thumb in general. I don't see a change in ARMCallLowering.cpp for GlobalISel ARM, to understand what needs to be done for Thumb (maybe I am looking at the wrong place)
I think that might be for the best.

The change would be in ARMCallLowering::lowerCall, where you set the opcode for the call (at the moment we only choose between ARM-specific opcodes). It should be easy to update the arm-call-lowering.ll test to account for that, but if we want to be thorough about testing we should also update arm-param-lowering.ll, and for that you'd also need to fix ARMCallLowering::lowerFormalArguments (I'm not sure off the top of my head, but it might be enough to just remove the check for Thumb and let the CCAssignFn work its magic).

In any case, that's probably enough work for a patch in its own right.


https://reviews.llvm.org/D39592