This is repeat of the legalizer tests done for ARM mode.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Hi Javed,
Is there a reason why you're not just adding a RUN line to the existing test file?
Also, could you have a look at arm-legalize-fp.mir and arm-legalize-divmod.mir while you're at it?
Thanks,
Diana
Comment Actions
Hi Diana.
Yes I too was thinking of adding just the run line, but I don't know when we move to instruction-selection would/could we do the same.
And if not, the separation upfront might be a good idea. Please advise. I am ok either way.
Yes, I will have a look at arm-legalize-fp.mir and arm-legalize-divmod.mir
Thanks a lot.
Comment Actions
I think it would be simpler to add a RUN line to the existing test (you can rename it to just legalizer.mir if you think the current name would be misleading).
It's ok to have separate tests for ARM vs Thumb instruction selection (*), since there would be more differences there, but I think for simple passes like the legalizer and reg bank select it's easier to follow if we keep them together in the same file.
(*) Alternatively, we can split up the instruction selection tests into smaller chunks and make them mixed ARM/Thumb if it turns out to be easier to read that way.
Comment Actions
Added thumb tests to all three. Also renamed the files to make it clear the test run both arm and thumb
Comment Actions
It seems the thumb instruction selection will take more work and needs probably split up into smaller ones (as discussed with Diana). So they will come later.
| test/CodeGen/ARM/GlobalISel/legalize-divmod.mir | ||
|---|---|---|
| 5 | Thumb division is actually controlled by the hwdiv feature, not by hwdiv-arm. The test should expose that behaviour (you'll also need to update the LegalizerInfo code to make it pass, but it's going to be very similar to what already exists for ARM). | |
| 53 | Hmm, this shouldn't be a BL for Thumb. You'll have to update ARMCallLowering to handle Thumb before we can test this properly. | |
Comment Actions
Thanks Diana for the review.
| test/CodeGen/ARM/GlobalISel/legalize-divmod.mir | ||
|---|---|---|
| 5 | Thanks for this. | |
| 53 | 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) | |
| lib/Target/ARM/ARMLegalizerInfo.cpp | ||
|---|---|---|
| 60 | 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()) | |
| test/CodeGen/ARM/GlobalISel/legalize-divmod.mir | ||
| 53 | 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. | |
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())