[PATCH] D56520: [WebAssembly] Mask SIMD shift values

Heejin Ahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 11 15:16:29 PST 2019


aheejin added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1170
+                                          Op.getOperand(0), MaskedShift)
+                                  .getNode());
+  }
----------------
Minor thing: It's sometimes hard to remember which is operand 0 and which is 1.. can we have inline comment for some of arguments to `DAG.getNode` and `DAG.UnrollVectorOp`, something like D50277? (It is a not landed yet CL of mine, and I brought this just for an example, but I saw this kind of commenting many other places in LLVM too)

And also we can early-exit i32/i64 case by 
```
// 32-bit and 64-bit unrolled shifts will have proper semantics
if (Op.getSimpleValueType().getVectorElementType().bitsGE(MVT::i32))
  return DAG.UnrollVectorOp(Op.getNode());

// Mask the operand to get proper semantics from 32-bit shift
SDValue Shift = Op.getOperand(1);
...
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56520





More information about the llvm-commits mailing list