[PATCH] D33247: [InstCombine] add isCanonicalPredicate() helper function and use it; NFCI

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineInternal.h:95-96
+    return true;
+  }
+}
+
----------------
davide wrote:
> spatel wrote:
> > davide wrote:
> > > I think GCC might complain about this so you might consider adding an `llvm_unreachable` at the end of this.
> > Do you know of a case where that happens? I couldn't find one:
> > https://godbolt.org/g/l0XnXd
> > ...so I'd rather not ugly up the code for something if it doesn't really happen. We could 'default: break' and return true outside of the switch if you think that reads better.
> I saw it emitting warning in similar cases, but not this one (just checked).
> If neither compiler complains, I think you can leave as is (worst case we'll need to fix a builbot failure, not a big deal).
I think gcc's complaint is normally when the switch covers all values of an enum and there is no default case.

From the coding standards:

"A knock-on effect of this stylistic requirement is that when building LLVM with GCC you may get warnings related to “control may reach end of non-void function” if you return from each case of a covered switch-over-enum because GCC assumes that the enum expression may take any representable value, not just those of individual enumerators. To suppress this warning, use llvm_unreachable after the switch."


https://reviews.llvm.org/D33247