[PATCH] D91084: [SVE][CodeGen] Add the ExtensionType flag to MGATHER

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:938
+
+  uint64_t MaskForTy = 0ull;
+  switch (ScalarTy.getSimpleVT().SimpleTy) {
----------------
nit: Capitalise to ULL seems more common in LLVM.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:955
+  APInt Val;
+  if (ISD::isConstantSplatVector(N, Val)) {
+    return Val.getLimitedValue() == MaskForTy;
----------------
nit: unnecessary curly braces.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5665
 
+  // fold (and (masked_gather x)) -> (zext_masked_gather x)
+  if (auto *GN0 = dyn_cast<MaskedGatherSDNode>(N0)) {
----------------
I'm surprised this doesn't break anything for other targets like X86.

In any case I think this fold (and same for the `sext_masked_gather`) should be guarded under `TargetLoweringInfo::isVectorLoadExtDesirable`, so that the target can inspect the operation and type, to make a suitable decision.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11526
+      ExtVT == cast<MaskedGatherSDNode>(N0)->getMemoryVT() &&
+      ((!LegalOperations && !cast<MaskedGatherSDNode>(N0)->isVolatile()) ||
+       TLI.isLoadExtLegal(ISD::SEXTLOAD, VT, ExtVT))) {
----------------
is `!LegalOperations` necessary here? The combine for `zext_masked_gather` doesn't seem to need it.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11527
+      ((!LegalOperations && !cast<MaskedGatherSDNode>(N0)->isVolatile()) ||
+       TLI.isLoadExtLegal(ISD::SEXTLOAD, VT, ExtVT))) {
+    MaskedGatherSDNode *LN0 = cast<MaskedGatherSDNode>(N0);
----------------
Is this missing a `N0.hasOneUse()` ?


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

https://reviews.llvm.org/D91084