Page MenuHomePhabricator

GlobalISel: Add computeKnownBitsForTargetInstr
ClosedPublic

Authored by arsenm on Jan 22 2020, 7:41 AM.

Details

Summary

I think we can save the MRI argument from these since it's in
GISelKnownBits already, but currently not accessible.

Implementation deferred to avoid dependency on other patches.

Diff Detail

Event Timeline

arsenm created this revision.Jan 22 2020, 7:41 AM
dsanders added inline comments.Jan 29 2020, 10:46 AM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
472

This TODO hasn't quite been handled. It's also intended to cover the case where you have target specific instructions (e.g. AArch64::ADDWrr), pseudo-instructions, or target-specific generic opcodes.

To cover this case we also need to call TL.computeNumSignBitsForTargetInstr() for the default: case

481

FirstAnswer is 1 so this if-statement is already inside the std::max

482

Why update a variable that and return that rather than just returning the answer as per the other code paths?

arsenm marked 2 inline comments as done.Mar 22 2020, 12:00 PM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
472

This is called in the default case

482

This is following the structure of the DAG implementation for eventually plugging in the default handling with computeKnownBits

arsenm updated this revision to Diff 251923.Mar 22 2020, 3:15 PM

Handle the fallback to computeKnownBits, copied directly from the DAG version. This should really get a test, but I don't think there's any operation that is implemented that should hit this path

foad added a subscriber: foad.Mar 23 2020, 2:09 AM
foad added inline comments.
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
500–507

I've simplified the SelectionDAG version of this code a bit in 7cdbf1ed4b94259a3aad2a7575e928fa61b0e57e.

dsanders accepted this revision.Mar 23 2020, 9:45 AM

LGTM after syncing up with @foad's change to use countLeadingOnes() in DAGISel

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
482

Ok, that makes sense

This revision is now accepted and ready to land.Mar 23 2020, 9:45 AM