This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] peek through shuffles in ComputeNumSignBits (PR37549)
ClosedPublic

Authored by spatel on Oct 24 2018, 10:23 AM.

Details

Summary

The motivating case is from PR37549:
https://bugs.llvm.org/show_bug.cgi?id=37549

The analysis improvement allows us to form a vector 'select' out of bitwise logic (the use of ComputeNumSignBits was added at rL345149).

The smaller test shows another InstCombine improvement - we use ComputeNumSignBits to add 'nsw' to shift-left.

Diff Detail

Event Timeline

spatel created this revision.Oct 24 2018, 10:23 AM
RKSimon added inline comments.Oct 24 2018, 10:41 AM
lib/Analysis/ValueTracking.cpp
2515

Do we know for certain that we don't reference an under operand at this point? SelectionDAG::ComputeNumSignBits bails i such cases.

spatel added inline comments.Oct 24 2018, 10:57 AM
lib/Analysis/ValueTracking.cpp
2515

I saw that, but I wasn't sure why bailing was necessary (in IR at least).

If the shuffle's mask contains an undef, then the result for that lane is undef:
http://llvm.org/docs/LangRef.html#shufflevector-instruction

So we can choose 0 or -1 for that undef element, and so it can't reduce the number of sign bits?

efriedma added inline comments.
lib/Analysis/ValueTracking.cpp
2515

We can pick any value for an undef, but every optimization has to handle it consistently. So you can't assume it's 0 unless you actually replace it with zero.

In this particular case, instead of looking for UndefValues, could you examine the mask instead?

spatel added inline comments.Oct 24 2018, 11:32 AM
lib/Analysis/ValueTracking.cpp
2515

IIUC, there are 2 independent factors here:

  1. We need to examine the mask for undef values and bail out if it contains any.
  2. If we look at both vector inputs to the shuffle, then we must take the minimum signbits value of those 2 values (we already do that in the DAG version). A potential complication for this enhancement is that I'm not sure if we handle an undef input here. That's why I made that case a TODO in this patch.
spatel updated this revision to Diff 170969.Oct 24 2018, 1:34 PM

Patch updated:
Don't try to analyze a shuffle with undef mask elements.
The negative test for 'nsw' actually demonstrates the bug in the last draft. If we had applied 'nsw' in that case, some other analysis could see poison for the undef lane that didn't exist in the original code.

RKSimon accepted this revision.Oct 26 2018, 5:31 AM

LGTM with one minor

lib/Analysis/ValueTracking.cpp
2520

assert that both ops aren't undef?

This revision is now accepted and ready to land.Oct 26 2018, 5:31 AM
This revision was automatically updated to reflect the committed changes.