This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: ComputeNumSignBits from load range metadata
AcceptedPublic

Authored by arsenm on Nov 15 2022, 5:25 PM.

Details

Summary

We're missing SimplifyDemandedBits styles of optimizations,
so one case differs from the DAG from not trimming the constant.
The other case is an optimization we get that the DAG doesn't do to
split the 64-bit shift.

Diff Detail

Event Timeline

arsenm created this revision.Nov 15 2022, 5:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2022, 5:25 PM
arsenm requested review of this revision.Nov 15 2022, 5:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2022, 5:25 PM
Herald added a subscriber: wdng. · View Herald Transcript
This revision is now accepted and ready to land.Nov 15 2022, 5:35 PM
arsenm added inline comments.Nov 15 2022, 5:35 PM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
722

Probably should still factor this in

foad added inline comments.Nov 15 2022, 11:09 PM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
609

Needs a comment, and possibly a better name, since this is not just looking at the range metadata, it is also handling different kinds of load opcode.

630

How could we hit this?

722

If you can rely on MMO->getSizeInBits() == CR.getBitWidth() then computeNumSignBitsFromRangeMetadata has already done this.

arsenm added inline comments.Nov 15 2022, 11:13 PM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
630

The anyextload case (or if the metadata was on a casted to vector result type)

arsenm updated this revision to Diff 480165.Dec 5 2022, 11:00 AM

Add comment

aemerson added inline comments.Dec 5 2022, 11:22 AM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
706–707

This can now be return TyBits - Ld->getMemSizeInBits() + 1;

721–722

ditto

arsenm updated this revision to Diff 480192.Dec 5 2022, 12:02 PM

Use getMemSizeInBits

foad added inline comments.Dec 5 2022, 10:10 PM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
630

I don't understand the "casted to vector" case - is there a test case for that?

633

Can this just go in the default case above?

689

What exactly is this check for?

arsenm added inline comments.Dec 7 2022, 1:11 PM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
630

It's the case where every load is legalized to an s32 (which I'd like to stop doing eventually). Apparently it's not reachable in the globalisel version since we don't handle the extract_vector_elt look through yet

633

This was to defend against the vector case, TyBits is the scalar size

689

You're only looking for element 0 if it's a vector result type

arsenm updated this revision to Diff 481033.Dec 7 2022, 1:11 PM

Add mir test (although kind of pointless, we can't hit this case here yet). Also copy the endianness check from the DAG version