Page MenuHomePhabricator

[X86] Do not allow FixupSetCC to relax constraints
ClosedPublic

Authored by hvdijk on Sun, Nov 22, 8:33 AM.

Details

Summary

The build bots caught two additional pre-existing problems exposed by the test change part of my change https://reviews.llvm.org/D91339, when expensive checks are enabled. https://reviews.llvm.org/D91924 fixes one of them, this fixes the other.

FixupSetCC will change code in the form of

%setcc = SETCCr ...
%ext1 = MOVZX32rr8 %setcc

to

%zero = MOV32r0
%setcc = SETCCr ...
%ext2 = INSERT_SUBREG %zero, %setcc, %subreg.sub_8bit

and replace uses of %ext1 with %ext2.

The register class for %ext2 did not take into account any constraints on %ext1, which may have been required by its uses. This change ensures that the original constraints are honoured, by instead of creating a new %ext2 register, reusing %ext1 and further constraining it as needed. This requires a slight reorganisation to account for the fact that it is possible for the constraining to fail, in which case no changes should be made.

Diff Detail

Event Timeline

hvdijk created this revision.Sun, Nov 22, 8:33 AM
hvdijk updated this revision to Diff 306936.Sun, Nov 22, 12:23 PM
hvdijk added a subscriber: RKSimon.

As requested by @RKSimon, added -verify-machineinstrs to test/CodeGen/X86/pic.ll so that the problem is visible even when expensive checks are disabled.

hvdijk updated this revision to Diff 306950.Sun, Nov 22, 5:49 PM

Fix FileCheck syntax at the same time, using CHECK-X32-DAG rather than CHECK-DAG-X32 that @pengfei noticed in D91339 after it had been committed already.

Fix FileCheck syntax at the same time, using CHECK-X32-DAG rather than CHECK-DAG-X32 that @pengfei noticed in D91339 after it had been committed already.

I pushed this fix in rG40783839e63a

RKSimon added a reviewer: glaubitz.
RKSimon added inline comments.Thu, Nov 26, 6:36 AM
llvm/lib/Target/X86/X86FixupSetCC.cpp
104–111

Add a comment for this early-out

hvdijk updated this revision to Diff 307929.Thu, Nov 26, 2:21 PM

Add a comment about skipping the optimization when constraining fails.

hvdijk marked an inline comment as done.Thu, Nov 26, 2:21 PM
hvdijk added inline comments.
llvm/lib/Target/X86/X86FixupSetCC.cpp
104–111

Sure, done.

RKSimon accepted this revision.Sat, Nov 28, 9:01 AM

LGTM - cheers.

If possible, please commit the pic.ll fix as a separate followup.

This revision is now accepted and ready to land.Sat, Nov 28, 9:01 AM
This revision was landed with ongoing or failed builds.Sat, Nov 28, 9:47 AM
This revision was automatically updated to reflect the committed changes.
hvdijk marked an inline comment as done.

If possible, please commit the pic.ll fix as a separate followup.

Sure, will do.