Page MenuHomePhabricator

[SelectionDAG][RISCV] Teach ComputeNumSignBits to handle SREM.

Authored by craig.topper on Feb 20 2021, 6:22 PM.



This also removes a pattern from RISCV that is no longer needed
since the sexti32 on the LHS of the srem in the pattern implies
the result is sign extended so the sign_extend_inreg should be
removed in DAG combine now.

Diff Detail

Unit TestsFailed

510 msx64 debian > MemProfiler-x86_64-linux.TestCases::test_new_load_store.cpp
Script: -- : 'RUN: at line 5'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/memprof/TestCases/test_new_load_store.cpp -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/memprof/X86_64LinuxConfig/TestCases/Output/test_new_load_store.cpp.tmp

Event Timeline

craig.topper created this revision.Feb 20 2021, 6:22 PM
craig.topper requested review of this revision.Feb 20 2021, 6:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2021, 6:22 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
luismarques accepted this revision.Feb 21 2021, 4:42 AM

LGTM. Nice!

This revision is now accepted and ready to land.Feb 21 2021, 4:42 AM
RKSimon added inline comments.Feb 21 2021, 6:05 AM

This is a lot more relaxed than ValueTracking ?

luismarques added inline comments.Feb 21 2021, 7:06 AM

Interesting! ValueTracking seems to abort on !Denominator->isStrictlyPositive(). When I reviewed the logic of this code at first I thought there would be cases where it would break, but when I thought about it more deeply it all seemed fine. Now I'm curious about what may explain the divergence. Maybe the implementation in ValueTracking was considering modulus instead of remainder?

craig.topper added inline comments.Feb 21 2021, 10:49 AM

The implementation in value tracking only handles constant right hand side. Looking at the history, its original implementation was incorrect and maybe thought the result was always positive at least from the the original comment.

I didn't handle constant here because I figured most targets would expand an srem by constant using BuildSDIV in TargetLowering.

craig.topper added inline comments.Feb 21 2021, 10:53 AM

It might make sense to update ValueTracking to hanldle the non-constant case. Some experiments I did from C seemed to get helped by CorrelatedValuePropagation. For example a shl after an srem with extra sign bits can pick up an nsw flag through CVP. This allows InstSimplify/InstCombine to late remover a shl+ashr sign extend pair due to the nsw.

RKSimon accepted this revision.Feb 21 2021, 11:04 AM
RKSimon added inline comments.

That sounds like a great followup but I'm happy for this to go ahead independently. Thanks for looking into this.

This revision was landed with ongoing or failed builds.Feb 21 2021, 11:17 AM
This revision was automatically updated to reflect the committed changes.