The patch essentially makes sure that X86CallLowering adds proper G_COPY/G_TRUNC and G_ANYEXT/G_COPY when we are doing lowering of arguments/returns for floating point values passed on registers. Tests are updated accordingly
Details
- Reviewers
aditya_nandakumar qcolombet - Commits
- rGda9e81c462fd: [GlobalISel][X86] Fixing failures after https://reviews.llvm.org/D37775
rGa85c4fc02916: [GlobalIsel][X86] Making {G_IMPLICIT_DEF, s128} legal
rL324665: [GlobalISel][X86] Fixing failures after https://reviews.llvm.org/D37775
rL324664: [GlobalIsel][X86] Making {G_IMPLICIT_DEF, s128} legal
Diff Detail
- Repository
- rL LLVM
Event Timeline
Note that I used getRegSizeInBits from https://reviews.llvm.org/D37775 for convenience. Which means that we need to apply this patch afterwards and this could be undesirable due to failures.
Other option would be to split getRegSizeInBits from D37775, but I'm open to other suggestions
Hi Alexander,
Thanks for looking into this.
General direction looks good. Couple of nits inlined, but most importantly, you should commit the autogenerated changes first to make the tests easier to review.
Thanks,
-Quentin
lib/Target/X86/X86CallLowering.cpp | ||
---|---|---|
140 ↗ | (On Diff #130572) | Could we refactor the code to avoid two similar call to buildCopy? [...] ExtReg = MIB->getOperand(0).getReg(); } else { ExtReg = extendRegister(ValVReg, VA); } |
264 ↗ | (On Diff #130572) | Shouldn't the added lines be here? |
lib/Target/X86/X86InstructionSelector.cpp | ||
666 ↗ | (On Diff #130572) | Mentioning both f32 and truncation in the same sentence is kind of strange to me, because f32 shouldn't use G_TRUNC but FPTRUNC. |
686 ↗ | (On Diff #130572) | Should we move these ![...]RC checks up? |
799 ↗ | (On Diff #130572) | We should make a helper function out of these lines and the previous chunk (canTurnIntoCOPY?). |
lib/Target/X86/X86LegalizerInfo.cpp | ||
169 ↗ | (On Diff #130572) | When is this pattern created? |
test/CodeGen/X86/GlobalISel/callingconv.ll | ||
3 ↗ | (On Diff #130572) | Could we keep the change in the name of a prefix a separate patch? |
329 ↗ | (On Diff #130572) | Do you know why this got better all of the sudden? |
404 ↗ | (On Diff #130572) | Why did the scheduling/RA change? |
test/CodeGen/X86/GlobalISel/irtranslator-callingconv.ll | ||
1 ↗ | (On Diff #130572) | Could do the autogeneration as a separate patch so that the changes are obvious? |
test/CodeGen/X86/GlobalISel/regbankselect-X86_64.mir | ||
1 ↗ | (On Diff #130572) | Ditto |
test/CodeGen/X86/GlobalISel/select-fconstant.mir | ||
1 ↗ | (On Diff #130572) | Ditto |
test/CodeGen/X86/GlobalISel/select-fsub-scalar.mir | ||
1 ↗ | (On Diff #130572) | Ditto |
test/CodeGen/X86/GlobalISel/select-memop-scalar.mir | ||
1 ↗ | (On Diff #130572) | Ditto |
test/CodeGen/X86/GlobalISel/x86_64-legalize-GV.mir | ||
2 ↗ | (On Diff #130572) | What is the rational for separating the two variants? |
Hi Quentin, first of all thanks for the review.
I realize my mistake about the tests, but not clear what would be the best direction.
Should I submit autogenerated tests that are not FP/G_COPY/G_TRUNC related, but that still need to be updated for D37775 as a separate patch? Should I have a review for it or can I just submit?
As for the tests that are affected by that patch, should I keep them here, even though they are autogenerated?
lib/Target/X86/X86InstructionSelector.cpp | ||
---|---|---|
686 ↗ | (On Diff #130572) | I'm not sure, to be honest. Added lines should be before if (DstRB.getID() != X86::GPRRegBankID) return false; Cause otherwise we will return in that check. At the same time those checks that you mention are after X86::GPRRegBankID one, so that move will change the logic. And I was trying to avoid changing the logic as I'm not that familiar with that code yet |
test/CodeGen/X86/GlobalISel/callingconv.ll | ||
329 ↗ | (On Diff #130572) | No, this change is not connected with my patch |
404 ↗ | (On Diff #130572) | That's again not connected |
Should I submit autogenerated tests that are not FP/G_COPY/G_TRUNC related, but that still need to be updated for D37775 as a separate patch? Should I have a review for it or can I just submit?
I would just push the autogenerate checks first directly (no review needed).
That way, the tests will only have the FP/COPY/ANYEXT changes + their impact on autogenerated checks.
lib/Target/X86/X86InstructionSelector.cpp | ||
---|---|---|
686 ↗ | (On Diff #130572) | I meant only the part that checks that the RegisterClasses are not nullptr. |
test/CodeGen/X86/GlobalISel/callingconv.ll | ||
329 ↗ | (On Diff #130572) | Ok, hopefully this will disappear when you rebase your patch after pushing the autogenerated checks. |
test/CodeGen/X86/GlobalISel/x86_64-legalize-GV.mir | ||
---|---|---|
2 ↗ | (On Diff #130572) | The reason was that MIR generated from the given IR for x86 and x86_64 is different. Namely the last two lines of MIR: %eax = COPY %0(p0) RET 0, implicit %eax For X86_64 it is: %rax = COPY %0(p0) RET 0, implicit %rax I was not sure about the best approach here, but splitting seemed reasonable, because we have different input MIR. As far as I see, just removing MIR part at all and relying on IR is not a desirable approach as it is less robust generally |
lib/Target/X86/X86LegalizerInfo.cpp | ||
---|---|---|
169 ↗ | (On Diff #130572) | It can be seen in regbankselect-X86_64.mir: define float @test_undef3() { ret float undef } before legalizer: %1(s128) = G_IMPLICIT_DEF %xmm0 = COPY %1(s128) RET 0, implicit %xmm0 after legalizer: %1:_(s128) = G_IMPLICIT_DEF %xmm0 = COPY %1(s128) RET 0, implicit %xmm0 |
Hi Quentin,
I hopefully addressed all your comments
I found a bug in the initial patch, where in IncomingValueHandler:assignValueToReg we go in the new code, instead of SExt/Zext cases and we end up with G_ANYEXT, instead of G_ZEXT/G_SEXT. I made a condition stronger and added the checks whether the LocSize and ValSize are equal. For those cases we expect to have a normal flow
LGTM with couple of nits below.
lib/Target/X86/X86CallLowering.cpp | ||
---|---|---|
258 ↗ | (On Diff #131056) | I would have expected the opposite in the comment: |
lib/Target/X86/X86InstructionSelector.cpp | ||
799 ↗ | (On Diff #130572) | I believe we could share some more code:
|
lib/Target/X86/X86LegalizerInfo.cpp | ||
169 ↗ | (On Diff #130572) | Got it, however, that could be a separate patch with its own test, right? |
Thanks for the review. The patch has to wait for D37775 getRegSizeInBits function implementation though, can it be submitted as a separate patch?
I can push that in a separate patch as soon as I get an LGTM.
Thanks for looking at this.
The getRegSIzeInBits function has already landed and now you should be able to get this through.