This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][X86] Fixing failures after https://reviews.llvm.org/D37775
ClosedPublic

Authored by aivchenk on Jan 19 2018, 2:11 AM.

Details

Summary
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

Diff Detail

Repository
rL LLVM

Event Timeline

aivchenk created this revision.Jan 19 2018, 2:11 AM

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?
E.g.,
unsigned ExtReg;
if (PhysRegSize > ValSize) {

[...]
ExtReg = MIB->getOperand(0).getReg();

} else {

ExtReg = extendRegister(ValVReg, VA);

}
MIRBuilder.buildCopy(PhysReg, ExtReg);

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.
Instead, I believe we want to say something like, if the value lives on the vector bank and goes into the floating bank, ...

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?).
I believe the only difference if swapping DstRC and SrcRC.

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?
(You can apply it before or after, up to you)

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.
If they are nullptr, the checks you're adding won't match anyway :).

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.

aivchenk added inline comments.Jan 22 2018, 12:56 AM
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

aivchenk updated this revision to Diff 131056.Jan 23 2018, 6:37 AM
aivchenk edited the summary of this revision. (Show Details)
aivchenk marked 6 inline comments as done.
aivchenk added inline comments.
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
aivchenk marked an inline comment as done.Jan 23 2018, 8:18 AM

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

qcolombet accepted this revision.Jan 25 2018, 6:09 PM

LGTM with couple of nits below.

lib/Target/X86/X86CallLowering.cpp
258 ↗(On Diff #131056)

I would have expected the opposite in the comment:
If LocSize and ValSize are *not* equal

lib/Target/X86/X86InstructionSelector.cpp
799 ↗(On Diff #130572)

I believe we could share some more code:

  • Turn this whole code sequence as selectTurnIntoCopy (or something) (BTW the refactoring for canTurnIntoCOPY looks good).
  • The expansion in trunc would simply turn into a call to that method
  • The expansion in any_ext would do a the same with the swap argument
lib/Target/X86/X86LegalizerInfo.cpp
169 ↗(On Diff #130572)

Got it, however, that could be a separate patch with its own test, right?

This revision is now accepted and ready to land.Jan 25 2018, 6:09 PM

Thanks for the review. The patch has to wait for D37775 getRegSizeInBits function implementation though, can it be submitted as a separate patch?

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.

Pushed the getSizeInBits change in r324125

Thanks for the review. The patch has to wait for D37775 getRegSizeInBits function implementation though, can it be submitted as a separate patch?

The getRegSIzeInBits function has already landed and now you should be able to get this through.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.