[PATCH] D56457: [WebAssembly][FastISel] Do not assume naive CmpInst lowering

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFastISel.cpp:451
+    // not a DAG ISel fallback. TODO: Recursively examine selects, phis, and,
+    // or, xor, constants.
     if (From == MVT::i1 && V != nullptr) {
----------------
tlively wrote:
> aheejin wrote:
> > Maybe I'm lacking the context. How does this code check if this is lowered by FastISel of DAG ISel fallback?
> > And real nit: Can we place the TODO in the next line? 
> I do not know of a way to tell whether or how something has already been lowered, so I just removed the optimization. I suppose to be safe I should remove the Argument optimization as well, since I certainly can't prove that it's correct. This change makes us generate worse code in lots of places, but since it is FastISel perhaps it doesn't matter so much?
I just removed the TODO. It is irrelevant anyway if we don't figure out how to tell if something has been lowered by FastISel.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFastISel.cpp:452
+    // or, xor, constants.
     if (From == MVT::i1 && V != nullptr) {
+      if ((isa<Argument>(V) && cast<Argument>(V)->hasZExtAttr()))
----------------
aheejin wrote:
> This case clause is entered only when `From == MVT::i1`. Do we need to check it again here?
Nope, good catch.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56457/new/

https://reviews.llvm.org/D56457