This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add DAG combine to turn (setcc X, 1, setne) -> (setcc X, 0, seteq) if we can prove X is 0/1.
ClosedPublic

Authored by craig.topper on Jan 14 2021, 3:52 PM.

Details

Summary

If we are able to compare with 0 instead of 1, we might be able
to fold the setcc into a beqz/bnez.

Often these setccs start life as an xor that gets converted to
a setcc by DAG combiner's rebuildSetcc. I looked into a detecting
(xor X, 1) and converting to (seteq X, 0) based on boolean contents
being 0/1 in rebuildSetcc instead of using computeKnownBits. It was
very perturbing to AMDGPU tests which I didn't look closely at.
It had a few changes on a couple other targets, but didn't seem
to be much if any improvement.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 14 2021, 3:52 PM
craig.topper requested review of this revision.Jan 14 2021, 3:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 3:52 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
lenary added inline comments.Jan 15 2021, 8:36 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1853

Is this not a general purpose combine if you have the same boolean contents? Or does it hinder optimisations on other platforms with a wider variety of condition codes?

craig.topper added inline comments.Jan 15 2021, 10:50 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1853

I tried to make it generic and failed a bunch of AMDGPU test. The majority where an assertion in SITargetLowering::LowerBRCOND that it expects control flow intrinsics results to be used by a setne with 1. Which this patch broke. I restricted the code to ignore setcc's with MVT::i1 inputs which avoids this assert. This left 12 failing tests.

Some of those tests got worse because SIOptimizeExecMaskingPreRA::optimizeVcndVcmpPair was optimizing the setne sequence but there's nothing to optimize the code with seteq. The comments specifically mention DAGCombiner::visitBRCOND and rebuildSetCC. This accounts for at least 4 of the 12. I stopped looking after that.

The was one test changed on AArch64 but it didn't reduce instruction count. A few tests changed on X86. One decreased instruction count. One went from a couple scalar instruction to some vector sequence(the IR contained vectors that were being simplified). Not sure what happened.

LGTM

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1853

Ok, cool.

I think it might be worth proposing as a target-independent patch and seeing what people say, but that shouldn't block us landing this to the RISC-V target.

lenary accepted this revision.Jan 19 2021, 8:40 AM
This revision is now accepted and ready to land.Jan 19 2021, 8:40 AM