This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] Simplify ISD::AND in GetDemandedBits.
ClosedPublic

Authored by efriedma on Feb 28 2017, 1:49 PM.

Details

Summary

This helps in cases involving bitfields where an AND is exposed by legalization.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Feb 28 2017, 1:49 PM
RKSimon added inline comments.Feb 28 2017, 2:55 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7441

How often are cases from each operand occurring? Since we canonicalize constants to the RHS would it be performant to test that first?

spatel edited edge metadata.Feb 28 2017, 3:01 PM

It's not this patch's fault of course, but that set -> sbb transform looks like a regression for every CPU that I checked in Agner's tables (sbb has higher latency and/or less throughput).
Should we fix that first?

How often are cases from each operand occurring? Since we canonicalize constants to the RHS would it be performant to test that first?

The cases affected here are all AND with a constant mask. Should I special-case constants, or just rearrange order of the checks?

It's not this patch's fault of course, but that set -> sbb transform looks like a regression for every CPU that I checked in Agner's tables (sbb has higher latency and/or less throughput). Should we fix that first?

I took a brief look, but there isn't any obvious cause. Maybe some x86 combine interacting badly with the fact that AVX-512 makes i1 a legal type.

spatel added a comment.Mar 1 2017, 6:34 AM

I took a brief look, but there isn't any obvious cause. Maybe some x86 combine interacting badly with the fact that AVX-512 makes i1 a legal type.

No problem - I'm rummaging around down here, so I'll try to find it. I don't think bogus sbb lowering is worth holding up this patch. Once Simon's concern is addressed, this should be good.

RKSimon added inline comments.Mar 3 2017, 10:22 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
7441

Does this still work with a check using isConstOrConstSplat(V.getOperand(1)) ? If so I'd go with that as what you have right now is probably overkill.

efriedma updated this revision to Diff 90536.Mar 3 2017, 2:47 PM

Don't call ComputeMaskedBits; just check for a constant.

RKSimon accepted this revision.Mar 4 2017, 3:42 PM

LGTM with one minor

test/CodeGen/X86/illegal-bitfield-loadstore.ll
80

Please can you remove the old ACHECK lines? The update script won't touch comments it doesn't recognise, including unknown filecheck prefixes.

This revision is now accepted and ready to land.Mar 4 2017, 3:42 PM
This revision was automatically updated to reflect the committed changes.