[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:
> bjope wrote:
> > 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.
> Then this is even more confusing than I thought! 
> It's fine if you want to leave any more changes to other patches. You've probably stepped through this more than anyone else by now. :)
I made a follow-up patch related to this here D117406.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117104