Page MenuHomePhabricator

[X86] Fix pattern match for 32-to-64-bit zext in the presence of AssertSext
ClosedPublic

Authored by mkuper on Nov 5 2014, 1:13 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 15796.Nov 5 2014, 1:13 AM
mkuper retitled this revision from to [X86] Fix pattern match for 32-to-64-bit zext in the presence of AssertSext.
mkuper updated this object.
mkuper edited the test plan for this revision. (Show Details)
mkuper added reviewers: spatel, rsmith, nadav.
mkuper added a subscriber: Unknown Object (MLST).
mkuper updated this revision to Diff 15797.Nov 5 2014, 1:18 AM

Uploaded diff with proper context, sorry about the first one.

spatel edited edge metadata.Nov 5 2014, 8:51 AM

Michael,
Thanks for working on this. If your patch doesn't cause any existing regression tests to fail, then you got further than I did. :)
It seems like a good solution to me, but let's have someone more familiar with the x86 backend take a look...or any backend? You said this bug also affects AArch64 and MSP430?

AArch64 and MSP have very similar patterns in their TD files - I guess copied from x86 - so I suspect they have the same issue.
I don't know if they actually have the same bug, and I don't have either the hardware or the familiarity with their ISA to patch them, even though the actual patch ought to look the same.

Anyway, if you have any potential reviewers in mind (other than Nadav who's already on the review), feel free to suggest.

nadav edited edge metadata.Nov 5 2014, 11:39 AM

Michael,

Is there a way to reduce the size of the testcase? Do you need two basic blocks? Can you add additional check lines to make sure that the ‘mov’ that you are matching is the one you are expecting?

Thanks,
Nadav

Yes, you need two basic blocks - the bug happens when you have a CopyFromReg -> AssertSext -> Zext chain, so trivially merging the basic blocks would make the issue go away.
Is there a simpler way to generate a CopyFromReg?

Regarding the mov - the resulting assembly is basically this:

orq $-2, %rcx
movl %ecx, %eax
imulq $7, %rax
shrq $32, %rax
retq

I can match the dest of the movl going into the imulq.
Would that be better?

On second thought, I could just get rid of most things after the zext - they were good for the reproducer, but probably not needed for the testcase.
The or before the first trunc is a must as that's what causes the AssertSext to be valid.
I'll try to reduce the testcase and upload a new patch.

A quick comment:
Please do not use date in the filename for the test case.

Thanks,
-Quentin

mkuper updated this revision to Diff 15845.Nov 6 2014, 1:36 AM
mkuper edited edge metadata.

Shortened the testcase and changed the name.
I think checking for a mov with the specific registers is the right thing to do, as the registers are determined by the ABI - the function parameter must come in ecx, and the ret value must go to eax.
Nadav, if you think that's too specific, let me know.

Michael

nadav added a comment.Nov 6 2014, 12:23 PM

Can you please add a check line for ‘ret’ ? I want to make sure that the check line does not mix with code from the following test. With that the code LGTM.

mkuper closed this revision.Nov 10 2014, 12:31 PM
mkuper updated this revision to Diff 16005.

Closed by commit rL221626 (authored by @mkuper).