This is an archive of the discontinued LLVM Phabricator instance.

[MIR-Canon] Hardening propagateLocalCopies.
ClosedPublic

Authored by plotfi on May 30 2019, 6:12 PM.

Details

Summary
[MIR-Canon] Hardening propagateLocalCopies.

- If there is no register class for a COPY's src or dst, bail.
- Fixes uses iterator clobbering bug.

Diff Detail

Event Timeline

plotfi created this revision.May 30 2019, 6:12 PM
compnerd added inline comments.May 30 2019, 6:33 PM
llvm/lib/CodeGen/MIRCanonicalizerPass.cpp
345

Can you explain why the type along isn't sufficient

346

I think that if (!MRI.getRegClassOrNull(Dst)) is more LLVM style

plotfi marked an inline comment as done.May 30 2019, 6:59 PM
plotfi added inline comments.
llvm/lib/CodeGen/MIRCanonicalizerPass.cpp
345

Because in some cases like:

%namedVReg4353:sreg_32_xm0 = S_MOV_B32 0
%namedVReg1352:vgpr_32 = COPY %namedVReg4353
...
BUFFER_STORE_DWORD_ADDR64 %namedVReg1352, %namedVReg1353, %namedVReg1354, 0, 0, 0, 0, 0, 0, implicit $exec

Folding the copy causes a verifier error even though the types are the same:

  %namedVReg4353:sreg_32_xm0 = S_MOV_B32 0
  ...
  BUFFER_STORE_DWORD_ADDR64 %namedVReg4353:sreg_32_xm0, %namedVReg1353:vreg_64, %namedVReg1354:sgpr_128, 0, 0, 0, 0, 0, 0, implicit $exec
  S_ENDPGM 0

*** Bad machine code: Illegal virtual register for instruction ***
- function:    foo
- basic block: %bb.0  (0x559c6b1fa878)
- instruction: BUFFER_STORE_DWORD_ADDR64 %namedVReg4353:sreg_32_xm0, %namedVReg1353:vreg_64, %namedVReg1354:sgpr_128, 0, 0, 0, 0, 0, 0, implicit $exec
- operand 0:   %namedVReg4353:sreg_32_xm0
Expected a VGPR_32 register, but got a SReg_32_XM0 register
LLVM ERROR: Found 1 machine code errors.

I think the right way is only to fold on matching types if regbankselect has not happened (in the case where there is no register class), which I will probably add in a different commit.

plotfi updated this revision to Diff 202357.May 30 2019, 7:54 PM

Adding some better comments. Addressing @compnerd's style suggestion.

plotfi updated this revision to Diff 202358.May 30 2019, 7:55 PM

whitespace

plotfi marked 2 inline comments as done.May 30 2019, 7:58 PM
plotfi updated this revision to Diff 202364.May 30 2019, 9:24 PM

test case enhancement

compnerd accepted this revision.May 30 2019, 9:30 PM

Thanks for adding the test case!

This revision is now accepted and ready to land.May 30 2019, 9:30 PM
This revision was automatically updated to reflect the committed changes.
spatel added a subscriber: spatel.Jun 9 2019, 7:18 AM

I'm seeing intermittent failures locally for this test on macOS since this was committed, and that seems confirmed by this bot:
http://green.lab.llvm.org/green/blue/organizations/jenkins/clang-stage1-cmake-RA-incremental/activity
(click 'Show More' at the bottom to extend the history and see this test as the only toggling failure with seemingly random NFC changes)

spatel added a comment.Jun 9 2019, 7:42 AM

I'm seeing intermittent failures locally for this test on macOS since this was committed, and that seems confirmed by this bot:
http://green.lab.llvm.org/green/blue/organizations/jenkins/clang-stage1-cmake-RA-incremental/activity
(click 'Show More' at the bottom to extend the history and see this test as the only toggling failure with seemingly random NFC changes)

When it fails, the verbose output says something like this:

: 'RUN: at line 1';   /Users/spatel/GitHub/llvm-project/release/bin/llc -o -  -march=amdgcn  -run-pass mir-canonicalizer  -x mir /Users/spatel/GitHub/llvm-project/llvm/test/CodeGen/MIR/AMDGPU/mir-canon-multi.mir | /Users/spatel/GitHub/llvm-project/release/bin/FileCheck /Users/spatel/GitHub/llvm-project/llvm/test/CodeGen/MIR/AMDGPU/mir-canon-multi.mir
--
Exit Code: 1

Command Output (stderr):
--
/Users/spatel/GitHub/llvm-project/llvm/test/CodeGen/MIR/AMDGPU/mir-canon-multi.mir:5:15: error: CHECK-NEXT: is not on the line after the previous match
# CHECK-NEXT: %namedVReg1358:vgpr_32 = COPY %namedVReg1361
              ^
<stdin>:77:2: note: 'next' match was here
 %namedVReg1358:vgpr_32 = COPY %namedVReg1361
 ^
<stdin>:75:46: note: previous match ended here
 %namedVReg1352:vgpr_32 = COPY %namedVReg4353
                                             ^
<stdin>:76:1: note: non-matching line after previous match is here
 %namedVReg1353:vreg_64 = REG_SEQUENCE %namedVReg4354, %subreg.sub0, %namedVReg1352, %subreg.sub1
plotfi added a comment.Jun 9 2019, 7:57 AM

Very strange that the non-determinism is introduced after fixing the iterator invalidation. I will investigate and put something together tomorrow.

I have a fix. Will land shortly.

r363012 should address netbsd and the other bot failures.