[PATCH] D117104: [DAGCombine] Refactor DAGCombiner::ReduceLoadWidth. NFCI

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

There are a complicated set of conditions to handle the various patterns/inputs. I don't know if I have accounted for all of the combinations.
I wonder if we'd be better off breaking it up, but if you feel confident that it's correct, let's keep it. :)

See inline for some minor changes.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12098
 
-/// If the result of a wider load is shifted to right of N  bits and then
-/// truncated to a narrower type and where N is a multiple of number of bits of
-/// the narrower type, transform it to a narrower load from address + N / num of
+/// If the result of a wider load is shifted to right of C bits and then
+/// truncated to a narrower type and where C is a multiple of number of bits of
----------------
This sounds like it was originally written with a little-endian-only implementation.
How about generalizing to:
  /// If the result of a load is shifted/masked/truncated to an effectively 
  /// narrower type, try to transform the load to a narrower type and/or
  /// use an extending load.

And fix the capitalization? "DAGCombiner::reduceLoadWidth()"


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12120
   unsigned ShAmt = 0;
+  // A special case is when the least significant bits from the load is masked
+  // away, but using an AND rather than a right shift. HasShiftedOffset is used
----------------
is masked -> are masked


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12152
+    // ZEXTLOAD (we could potentially replace it by a more narrow SEXTLOAD
+    // followed by a ZEXT, but that it not handled at the moment).
+    if (LN->getExtensionType() == ISD::SEXTLOAD)
----------------
that it -> that is


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12181
+  // other reasons (e.g. based on Opc==TRUNCATE) and that is why some checks
+  // needs to be done here as well.
+  if (Opc == ISD::SRL || N0.getOpcode() == ISD::SRL) {
----------------
needs to be -> need to be


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12214
+    // Is the shift amount a multiple of size of ExtVT?
+    if ((ShAmt & (ExtVTBits-1)) != 0)
+      return SDValue();
----------------
This assumes that ExtVTBits is a power-of-2, but is that enforced/asserted?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12271-12272
 
   // For big endian targets, we need to adjust the offset to the pointer to
   // load the correct bytes.
   if (DAG.getDataLayout().isBigEndian())
----------------
This seems backwards. 
It's the little-endian target that needs to adjust the pointer. We're chopping off the LSB, so this is always converting ShAmt back to zero for big-endian?
fe17ce0fa6626f79be66


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117104