[PATCH] D159140: [GlobalISel] LegalizationArtifactCombiner: Elide redundant G_AND

[Tobias Stadler via Phabricator via llvm-commits]

tobias-stadler added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:143
+      // Elide AND if it is proven redundant (e.g between boolean uses and defs)
+      if (KB && (KB->getKnownZeroes(AndSrc) | ExtMaskVal).isAllOnes()) {
+        replaceRegOrBuildCopy(DstReg, AndSrc, MRI, Builder, UpdatedDefs,
----------------
tobias-stadler wrote:
> arsenm wrote:
> > tobias-stadler wrote:
> > > tobias-stadler wrote:
> > > > arsenm wrote:
> > > > > How much of the benefit is there from only handling the constant case?
> > > > I'm not quite sure what you mean by  "constant case". Do you mean after a single computeKnownBitsImpl calll? I can try that by reducing MaxDepth even more and will report. I believe most of the benefit comes from the boolean case. I'm not aware of a method to determine if we can ignore the AND other than how KnownBits does it using BooleanContents for applicable instructions, in which case I wouldn't want to duplicate this functionality here. Maybe we can add a MaxDepth override for cases like this?
> > > MaxDepth = 1 compared to MaxDepth = 6 doesn't seem to measurably affect compile-time, but geomean size..text goes from -5.6% to -5.5%. I don't think there's much to gain by adding extra complexity here just too avoid KnownBits.
> > I mean by the input is a literal G_CONSTANT or copy of G_CONSTANT
> I tried it out. Only checking when lookThroughCopyInstrs(AndSrc) is a G_CONSTANT brings no benefit at all. A simple example of G_ZEXT(G_TRUNC(G_CONSTANT)) is already combined away by the other combines in the artifact combiner. The only difference between me directly checking for G_CONSTANT in this combine is that we end up with one less stray COPY.
Sorry, I didn't phrase this clearly. I first ran the "constant-case-only" version against O0 CTMark, which didn't show any difference in code size and compile-time compared to the current main branch. I then ran a simple hand-coded G_ZEXT(G_TRUNC(G_CONSTANT)) example against both versions to make sure the additional check did the right thing, which confirmed that there shouldn't be much of a difference in the first place. I couldn't think of and haven't seen examples in the tests where a G_CONSTANT is truncated and then extended again in such a way that this combine would emit better code than the other combines (apart from removing some COPY instructions, which also happens in some of the test differences).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159140