This is an archive of the discontinued LLVM Phabricator instance.

[X86] Support `X86ISD::PCMPEQ` and `X86ISD::PCMPGT` in ComputeKnownBits
ClosedPublic

Authored by goldstein.w.n on Apr 21 2023, 2:33 PM.

Details

Summary

These functions where missing support but are used enough that it
makes sense to track them.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Apr 21 2023, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2023, 2:33 PM
goldstein.w.n requested review of this revision.Apr 21 2023, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2023, 2:33 PM
RKSimon accepted this revision.Apr 22 2023, 2:06 AM

Nice! LGTM with one minor

llvm/lib/Target/X86/X86ISelLowering.cpp
38573

(style) Don't use auto

This revision is now accepted and ready to land.Apr 22 2023, 2:06 AM
goldstein.w.n marked an inline comment as done.Apr 23 2023, 3:46 PM
This revision was landed with ongoing or failed builds.Apr 26 2023, 9:49 PM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Apr 28 2023, 12:50 AM

Shouldn't this be a DAGCombine instead? It seems pointless to make computeKnownBits() recurse past an instruction that can only return either unknown or constant. If it returns constant, we want to just replace the instruction, and otherwise we get zero information.

Shouldn't this be a DAGCombine instead? It seems pointless to make computeKnownBits() recurse past an instruction that can only return either unknown or constant. If it returns constant, we want to just replace the instruction, and otherwise we get zero information.

At least part of this is only matching cases because it has access to the DemandedElts mask

nikic added a comment.Apr 28 2023, 1:25 AM

Shouldn't this be a DAGCombine instead? It seems pointless to make computeKnownBits() recurse past an instruction that can only return either unknown or constant. If it returns constant, we want to just replace the instruction, and otherwise we get zero information.

At least part of this is only matching cases because it has access to the DemandedElts mask

Hm, through which pathway does this actually end up folding in the non-trivial DemandedElts case? I'd have guessed SimplifyDemandedBits, but it doesn't seem to have a generic constant known bits to constant combine.

@goldstein.w.n In case you missed it, this has somehow exposed a miscompile: https://github.com/llvm/llvm-project/issues/67287

@goldstein.w.n In case you missed it, this has somehow exposed a miscompile: https://github.com/llvm/llvm-project/issues/67287

Oh, had missed it. Looking into it.

llvm/test/CodeGen/X86/vec_setcc-2.ll