Page MenuHomePhabricator

[SystemZ] Bugfix in emitSelect()
ClosedPublic

Authored by jonpa on Feb 10 2020, 12:15 PM.

Details

Summary

When more than one SelectPseudo instruction is handled a new MBB is returned. This must not be done if that would result in leaving an undhandled isel pseudo behind in the original MBB.

Fixes https://bugs.llvm.org/show_bug.cgi?id=44849.

Diff Detail

Event Timeline

jonpa created this revision.Feb 10 2020, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2020, 12:16 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

BTW, this was an example where the backend messed up and left behind a pseudo MI (that uses custom insertion) after the FinalizeISel pass. This was a bit suprising to me as in my memory there was an assert for this that checked that all pseudos were gone after the loop over MF. Perhaps that would still be good to have..?

uweigand added inline comments.Feb 11 2020, 4:29 AM
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
6889

Isn't this always true for select pseudos? Why doesn't make the rest of this loop a no-op?

jonpa updated this revision to Diff 243860.Feb 11 2020, 7:20 AM
jonpa marked 2 inline comments as done.

Do the check after checking for a select pseudo. Moved down also the check for CC-def, since a SelectPseudo does not define CC.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
6889

Oops! Sorry.

uweigand accepted this revision.Feb 11 2020, 7:26 AM

I guess it would be nicer if we could still handle this case somehow.

But anyway, for now bailing out is certainly preferable to aborting compilation.

LGTM.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
6889

Interesting that this didn't cause any test to fail ... Don't we have tests that verify this loop does something?

This revision is now accepted and ready to land.Feb 11 2020, 7:26 AM
This revision was automatically updated to reflect the committed changes.
jonpa marked an inline comment as done.Feb 11 2020, 7:49 AM
jonpa added inline comments.
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
6889

We do and they were failing, it was just me who was too happy finding the cause of this problem and then forgetting to run the tests before submitting...

fhahn added a subscriber: fhahn.Mar 13 2020, 5:24 AM

It seems like the test added in this patch fails with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15376/testReport/junit/LLVM/CodeGen_SystemZ/multiselect_02_mir/

It would be great if you could take a look.

It seems like the test added in this patch fails with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15376/testReport/junit/LLVM/CodeGen_SystemZ/multiselect_02_mir/

It would be great if you could take a look.

This test has been running fine for a while with expensive checks enables, so I don't quite understand why this is failing on that particular build...

I tried to use the same cmake options as the Jenkins job did:

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;compiler-rt;libcxx" -DLLVM_ENABLE_LTO=Off -DLLVM_BUILD_EXAMPLES=On -DCMAKE_MACOSX_RPATH=On -DCMAKE_BUILD_TYPE=Release -DLLVM_BUILD_EXTERNAL_COMPILER_RT=On -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON -DLIBCXX_ENABLE_SHARED=OFF -DLIBCXX_ENABLE_STATIC=OFF -DLIBCXX_INCLUDE_TESTS=OFF -DLLVM_ENABLE_ASSERTIONS=On $LLVM_SRC/llvm

All tests are passing, on 7b74b0d (March 13).

I see in the Jenkins log "Checking out Revision 7eeee2a196c01bd43fa468339d39251faac1a88e (refs/remotes/origin/master)", but that is not a commit in my repo...

fhahn added a comment.Mar 14 2020, 9:54 AM

It seems like the test added in this patch fails with expensive checks enabled: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15376/testReport/junit/LLVM/CodeGen_SystemZ/multiselect_02_mir/

It would be great if you could take a look.

This test has been running fine for a while with expensive checks enables, so I don't quite understand why this is failing on that particular build...

I tried to use the same cmake options as the Jenkins job did:

cmake -G Ninja -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;compiler-rt;libcxx" -DLLVM_ENABLE_LTO=Off -DLLVM_BUILD_EXAMPLES=On -DCMAKE_MACOSX_RPATH=On -DCMAKE_BUILD_TYPE=Release -DLLVM_BUILD_EXTERNAL_COMPILER_RT=On -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON -DLIBCXX_ENABLE_SHARED=OFF -DLIBCXX_ENABLE_STATIC=OFF -DLIBCXX_INCLUDE_TESTS=OFF -DLLVM_ENABLE_ASSERTIONS=On $LLVM_SRC/llvm

All tests are passing, on 7b74b0d (March 13).

I see in the Jenkins log "Checking out Revision 7eeee2a196c01bd43fa468339d39251faac1a88e (refs/remotes/origin/master)", but that is not a commit in my repo...

Hm, it looks like it also failed on Marc 5th, and there have been no builds in between. (http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/15374/). Let's keep an eye out and see if it keeps failing.