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

Fri Jan 11 10:42:27 PST 2019

tlively marked an inline comment as done.
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) {
----------------
nikic wrote:
> tlively wrote:
> > 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.
> FastISel selects instructions in reverse order, so at this point the icmp hasn't been selected yet and there's no way to tell -- it would only be possible to replicate the checks FastISel does and ensure they'll all be true. That seems somewhat fragile though.
@sunfish, it seems to me that we do know for sure that i1 Arguments will always produce a 0 or a 1 because there is no way DAG ISel could combine away an Argument node. This is in contrast to the compare node, which could be combined away by DAG ISel. Does that seem right to you?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56457