Page MenuHomePhabricator

[GISel] Eliminate redundant bitmasking
ClosedPublic

Authored by jroelofs on Fri, May 28, 12:11 PM.

Details

Summary

This was a GISel vs SDAG regression that showed up at -Os in:
SingleSource/Benchmarks/Adobe-C++/simple_types_constant_folding.test on aarch64

https://llvm.godbolt.org/z/aecjodsjG

Diff Detail

Event Timeline

jroelofs created this revision.Fri, May 28, 12:11 PM
jroelofs requested review of this revision.Fri, May 28, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, May 28, 12:11 PM
paquette added inline comments.Fri, May 28, 12:25 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
660

Probably shouldn't be in this group since it doesn't use KnownBits?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3008

This doesn't use KnownBits?

(If this was yoinked from somewhere else, then we probably need to update a combine. GISelKnownBits supports vectors nowadays last time I checked.)

3025

Minor stylistic suggestion:

if (C1 & C2) {
  B.buildAnd(...);
  return;
}

B.buildCopy(...);

Is it actually necessary to emit the copy at all though? You could do

Dst = G_CONSTANT iN 0

right?

llvm/test/CodeGen/AArch64/GlobalISel/opt-overlapping-and.ll
1 ↗(On Diff #348565)

Can this be a MIR test?

jroelofs added inline comments.Fri, May 28, 12:42 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
660

Oops, yeah. I thought I was going to need KnownBits, but didn't end up using it.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3008
3025

Are we guaranteed that Dst is not a physical register? I wasn't sure.

paquette added inline comments.Fri, May 28, 1:26 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3025

With G_AND (and other generic opcodes) we can be certain that we only have vregs.

(You have to be careful with COPY instructions, but in this case, we don't have to worry.)

jroelofs updated this revision to Diff 348583.Fri, May 28, 2:07 PM
jroelofs added inline comments.Fri, May 28, 2:18 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3025

👍

llvm/test/CodeGen/AArch64/GlobalISel/opt-overlapping-and.ll
1 ↗(On Diff #348565)

👍

jroelofs updated this revision to Diff 348586.Fri, May 28, 2:29 PM

simplify test

jroelofs added inline comments.Fri, May 28, 2:45 PM
llvm/test/CodeGen/AArch64/GlobalISel/opt-overlapping-and.mir
4

This needs:

# REQUIRES: asserts

for the same reason it's needed in: https://reviews.llvm.org/D99283

paquette accepted this revision.Fri, May 28, 3:02 PM

LGTM with some minor comments

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3022

I think you can just do

B.buildConstant(Dst, 0);

and omit the replaceRegWith (similar to the buildAnd case above.)

llvm/test/CodeGen/AArch64/GlobalISel/opt-overlapping-and.mir
44

You shouldn't need the registers section in any of the tests here.

You can also drop

  • alignment
  • liveins
  • frameinfo
  • machineFunctionInfo

(Technically you can drop tracksRegLiveness too, but it's useful if you want to test the pipeline beyond the combiner.)

This revision is now accepted and ready to land.Fri, May 28, 3:02 PM
jroelofs updated this revision to Diff 348594.Fri, May 28, 4:22 PM
jroelofs added inline comments.Wed, Jun 16, 3:04 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
3022

When I tried it that way, it wasn't RAUW-ing Dst appropriately, and then the verifier complains:

*** Bad machine code: Reading virtual register without a def ***
- function:    bitmask_no_overlap
- basic block: %bb.0  (0x7fe69b06d6c8)
- instruction: $w0 = COPY %4:_(s32)
- operand 1:   %4:_
This revision was landed with ongoing or failed builds.Thu, Jun 17, 12:53 PM
This revision was automatically updated to reflect the committed changes.
foad added a subscriber: foad.Fri, Jun 18, 6:06 AM