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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
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();
----------------
spatel wrote:
> This assumes that ExtVTBits is a power-of-2, but is that enforced/asserted?
I've not really understood why these checks exist. I mean, if there is no SRL at all we won't even take this path. So the code below (I figure mainly `isLegalNarrowLdSt`) need to ensure the legality anyway for the more general situation. So maybe this is some kind of early out (possibly saving a tiny amount of compile time). Or it is protecting the AND masking below somehow (but I can't really see that the AND masking depend on these properties).

Nevertheless, just like you have spotted, these checks aren't even making sense when ExtVTBits isn't a power of two.

As it happens we do get here for multiple lit tests with ExtEVTBits not being a power-of-2.

If I simply remove the checks, then I get diffs in several lit tests. I did examined one such test, and it turned out that we ended up with some slightly different order of transforms (resulting in  `(and (load i32), 7) `  instead of `(and (sexload i8 to i32), 7)`, not really sure whichever would be better in that particular case). If the sextload is preferred, then I guess there should be another DAGCombine added that is doing such a transform(?).

If I instead bail out here if ExtVTBits isn't a power-of-2, then I get diffs in 3 lit tests. Those diffs looked like regressions as the instruction count increased in all three cases.

Skipping the tests seems like the better solution of the two above, but I'd rather fix that in a separate patch. And there might actually be some alternatives to explore. The AND mask hack below might detect that ExtVT can be reduced further into something that is a power-of-2, so bailing out on ExtVTBits not being a power-of-2 before the AND mask check was perhaps too restricting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117104