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

[xbolva00]

Summary

Found by PVS Studio

Not familiar with this code; no testcase.

[xbolva00]

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

[RKSimon]

LGTM by inspection

[xbolva00]

Thanks

[thakis]

Does this have an observable efefceffect that can be tested?

[RKSimon]

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).

[xbolva00]

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

[xbolva00]

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.

[xbolva00]

It seems that x86 is ok, aarch64 is broken too.

@t.p.northover http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/24174/steps/annotate/logs/stdio

[xbolva00]

Oh!

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

[simoncook]

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]

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

[kparzysz]

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]

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

[xbolva00]

@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.

[hans]

In D69741#1739377, @xbolva00 wrote:

@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.

Maybe someone from RISC-V could try to figure out what pattern it is that's causing the problem? Perhaps one approach could be to bisect over RISC-V commits with this patch applied and seeing where the timeouts start?

[xbolva00]

There is no official riscv buildbot? I think it is mandatory to have a buildbot for official (non experimental) target.

http://lab.llvm.org:8011/builders

[lenary]

In D69741#1748935, @xbolva00 wrote:

There is no official riscv buildbot? I think it is mandatory to have a buildbot for official (non experimental) target.

The RISC-V backend is now enabled by default, so general buildbot builders are building the RISC-V backend.

[xbolva00]

Any progress?

llvm/utils/TableGen/CodeGenDAGPatterns.cpp

bool TypeInfer::EnforceSmallerThan(TypeSetByHwMode &Small,
   // 1. Only allow integer or floating point types and make sure that
   //    both sides are both integer or both floating point.
   // 2. Make sure that either both sides have vector types, or neither
   //    of them does.
   for (unsigned M : Modes) {
     TypeSetByHwMode::SetType &S = Small.get(M);
     TypeSetByHwMode::SetType &B = Big.get(M);
 
-    if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr)) {
+    if (any_of(S, isIntegerOrPtr) && any_of(B, isIntegerOrPtr)) {
       auto NotInt = [](MVT VT) { return !isIntegerOrPtr(VT); };
       Changed |= berase_if(S, NotInt) |
                  berase_if(B, NotInt);
     } else if (any_of(S, isFloatingPoint) && any_of(B, isFloatingPoint)) {
       auto NotFP = [](MVT VT) { return !isFloatingPoint(VT); };
       Changed |= berase_if(S, NotFP) |
                  berase_if(B, NotFP);
     } else if (S.empty() || B.empty()) {