[PATCH] D53057: [WebAssembly] Handle undefined lane indices in SIMD patterns

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 2 inline comments as done.
tlively added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1032
+      // Lower undefs to zero
+      uint64_t MaskVal = Mask[I] == -1 ? 0 : (uint64_t)Mask[I] * LaneBytes + J;
+      Ops[OpIdx++] = DAG.getConstant(MaskVal, DL, MVT::i32);
----------------
aheejin wrote:
> aheejin wrote:
> > Why -1? Is it how `undef` is represented?
> I think `MaskVal` sounds confusing. It sounds like it is a mask value (such as 0xff). Can we change it to something else, like, just `Val` or something?
`ByteIndex` seemed like a nice descriptive name.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1032
+      // Lower undefs to zero
+      uint64_t MaskVal = Mask[I] == -1 ? 0 : (uint64_t)Mask[I] * LaneBytes + J;
+      Ops[OpIdx++] = DAG.getConstant(MaskVal, DL, MVT::i32);
----------------
tlively wrote:
> aheejin wrote:
> > aheejin wrote:
> > > Why -1? Is it how `undef` is represented?
> > I think `MaskVal` sounds confusing. It sounds like it is a mask value (such as 0xff). Can we change it to something else, like, just `Val` or something?
> `ByteIndex` seemed like a nice descriptive name.
Yes, that seems to be how `undef` is represented.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:194
+def : Pat<(and (i32 (vector_extract (v8i16 V128:$vec), undef)), (i32 0xffff)),
+          (EXTRACT_LANE_v8i16_u V128:$vec, 0)>;
+def : Pat<(i32 (vector_extract (v16i8 V128:$vec), undef)),
----------------
aheejin wrote:
> Are these unnecessary sequence of masking by `and` also removed in non-undef cases?
Yes, see multiclass `ExtractLaneExtended`, which uses the corresponding patterns in multiclass `ExtractPat`.


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:202
+def : Pat<(sext_inreg (i32 (vector_extract (v8i16 V128:$vec), undef)), i16),
+          (EXTRACT_LANE_v8i16_s V128:$vec, 0)>;
+def : Pat<(vector_extract (v4i32 V128:$vec), undef),
----------------
aheejin wrote:
> Here the same. Are these `sext_inreg` also removed in non-undef cases?
Yes, same as above.


Repository:
  rL LLVM

https://reviews.llvm.org/D53057