Page MenuHomePhabricator

[Codegen] Both sides of '&&' are same; fixed
AbandonedPublic

Authored by xbolva00 on Fri, Nov 1, 4:38 PM.

Details

Summary

Found by PVS Studio

Not familiar with this code; no testcase.

Diff Detail

Event Timeline

xbolva00 created this revision.Fri, Nov 1, 4:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 1, 4:38 PM

BTW, is anybody interested to help with fixing errs/warnings found by PVS Studio? List is huuuge.

xbolva00 edited the summary of this revision. (Show Details)Fri, Nov 1, 5:11 PM
RKSimon accepted this revision.Sat, Nov 2, 2:30 AM

LGTM by inspection

This revision is now accepted and ready to land.Sat, Nov 2, 2:30 AM
This revision was automatically updated to reflect the committed changes.

Does this have an observable efefceffect that can be tested?

RKSimon reopened this revision.Sat, Nov 2, 10:25 AM

This might be trickier than it looks as its causing buildbot timeouts due to infinite loop in some llvm-tblgen calls (RISCVGenGlobalISel.inc is the one I've noticed).

This revision is now accepted and ready to land.Sat, Nov 2, 10:25 AM
RKSimon requested changes to this revision.Sat, Nov 2, 10:25 AM
This revision now requires changes to proceed.Sat, Nov 2, 10:25 AM
xbolva00 added subscribers: simoncook, asb.EditedSat, Nov 2, 10:33 AM

Expired timeouts were sadly caused by other commit, after that commit was reverted, major buildbots are green.

RISCV folks should look at it if this exposed bugs..

cc @asb @simoncook

But http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/10228 is red due to this patch for sure.. sad.

Gonna revert this one.

Oh!

@krasimir tried to fix it too some time ago (https://reviews.llvm.org/D61632) and failed with same error - RISCV to blame :)

simoncook added inline comments.Tue, Nov 5, 3:53 AM
llvm/utils/TableGen/CodeGenDAGPatterns.cpp
482

I'm not familiar with this part of TableGen, but trying to narrow down what about RISCV causes this issue, it seems that patterns that have sext_inreg nodes cause this infinite loop, whereby this comparison now fails, and we fall into line 490 and remove modes.

Reading the comment above, it reads to me like this comparison should instead be if either S or B contains an integer mode, rather than if both do? If I change both this and line 486 to || rather than && in addition to this change then the infinite loop disappears. I ran make check-llvm with that change, and I don't see any regression tests failing.

xbolva00 marked an inline comment as done.Tue, Nov 5, 4:43 AM
xbolva00 added a subscriber: kparzysz.

This code was implemented by @kparzysz, please can you look at it?

kparzysz added inline comments.Thu, Nov 7, 8:44 AM
llvm/utils/TableGen/CodeGenDAGPatterns.cpp
482

The intent was what this change shows, i.e. if S contains an integer && B contains an integer). I don't know what's causing the timeouts, but IIRC the code at lines 505-509 used to cause some weirdness on X86 (it's been fixed since). Without knowing how the timeouts occur there isn't much that can be done.

xbolva00 abandoned this revision.Thu, Nov 7, 10:20 AM

Since nobody knows what should be done, where the bug is - Abandoning.

xbolva00 added a comment.EditedFri, Nov 8, 1:34 PM

@RKSimon / @hans - do you have any idea how to progress here? It is shame to have such obvious mistake in code just to keep RISC V working. Since no other targets are broken, I think this needs to be solved on RISC V side.

xbolva00 added a subscriber: hans.Fri, Nov 8, 1:36 PM