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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin 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) {
----------------
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? 


================
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()))
----------------
This case clause is entered only when `From == MVT::i1`. Do we need to check it again here?


================
Comment at: test/CodeGen/WebAssembly/bug-40172.ll:3
+
+; Regression test for bug 40172. The problem was that FastISel assumed
+; that CmpInst results did not need to be zero extended because
----------------
Nit: It seems people add bug-related test cases in the name of PRXXXXX.ll. (I'm not sure what PR stands for. Do you know?) Maybe better to follow that convention. So in this case it will be PR40172.ll. Fix the comment/CL description too.


================
Comment at: test/CodeGen/WebAssembly/bug-40172.ll:7
+; test case FastISel falls back to DAG ISel, which combines away the
+; comparison, validating FastISel's assumption.
+
----------------
invalidating?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56457