This is an archive of the discontinued LLVM Phabricator instance.

[MachineVerifier] Diagnose invalid INSERT_SUBREGs
ClosedPublic

Authored by jroelofs on Jul 13 2021, 6:11 PM.

Diff Detail

Event Timeline

jroelofs created this revision.Jul 13 2021, 6:11 PM
jroelofs requested review of this revision.Jul 13 2021, 6:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2021, 6:11 PM
jroelofs updated this revision to Diff 358473.Jul 13 2021, 6:14 PM

hm. This breaks: llvm/test/CodeGen/X86/domain-reassignment.mir

%9:gr32 = INSERT_SUBREG %8:gr32(tied-def 0), %17:gr16, %subreg.sub_8bit_hi_phony
jroelofs updated this revision to Diff 358477.Jul 13 2021, 6:26 PM

Fix llvm/test/CodeGen/X86/domain-reassignment.mir to not rely on magic constants.

arsenm added inline comments.Jul 14 2021, 1:16 PM
llvm/lib/CodeGen/MachineVerifier.cpp
1786

This is already TRI tin the verifier class

llvm/test/MachineVerifier/test_insert_subreg.mir
1

-global-isel doesn't do anything with -run-pass

jroelofs updated this revision to Diff 358737.Jul 14 2021, 1:58 PM
jroelofs marked 2 inline comments as done.
aemerson accepted this revision.Jul 15 2021, 10:05 AM
This revision is now accepted and ready to land.Jul 15 2021, 10:05 AM
This revision was landed with ongoing or failed builds.Jul 16 2021, 9:43 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jul 16 2021, 9:50 AM

This seems to break tests: http://45.33.8.238/linux/51314/step_12.txt

Please take a look.

Reverted for the moment while I work out what broke.

Thanks thakis!

jroelofs updated this revision to Diff 359486.Jul 16 2021, 5:00 PM

One of the two issues in the tests was easily fixable:
https://reviews.llvm.org/D106168

The other issue appears to be much trickier, as it stems from the ARM backend treating MPR regs as if they were ssub subregs. This causes a contradiction in sizes, since ssub's are 32 bits whereas MPR regs are fp16's.

jroelofs reopened this revision.Jul 16 2021, 5:15 PM
This revision is now accepted and ready to land.Jul 16 2021, 5:15 PM
This revision was landed with ongoing or failed builds.Jul 20 2021, 5:32 PM
This revision was automatically updated to reflect the committed changes.
uabelho added inline comments.
llvm/lib/CodeGen/MachineVerifier.cpp
1787

Don't you need to take operand 2's subreg into account here?

I'm seeing failures for cases where

MI->getOperand(2).getReg()

is indeed too big, but

MI->getOperand(2).getSubReg()

is specifying a subreg of a small enough size so there is no problem.

uabelho added inline comments.Jul 20 2021, 11:44 PM
llvm/lib/CodeGen/MachineVerifier.cpp
1787

This seems to work better for me:

unsigned InsertedSize;
if (unsigned SubIdx = MI->getOperand(2).getSubReg())
  InsertedSize = TRI->getSubRegIdxSize(SubIdx);
else
  InsertedSize =
    TRI->getRegSizeInBits(MI->getOperand(2).getReg(), *MRI);
jroelofs added inline comments.Jul 21 2021, 8:47 AM
llvm/lib/CodeGen/MachineVerifier.cpp
1787