This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] Allow scalable vectors in ComputeNumSignBits

Authored by reames on Oct 31 2022, 6:25 PM.



This is a continuation of the series of patches adding lane wise support for scalable vectors in various knownbit-esq routines.

The basic idea here is that we track a single lane for scalable vectors which corresponds to an unknown number of lanes at runtime. This is enough for us to perform lane wise reasoning on many arithmetic operations.

Diff Detail

Event Timeline

reames created this revision.Oct 31 2022, 6:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 6:25 PM
reames requested review of this revision.Oct 31 2022, 6:25 PM
reames updated this revision to Diff 476483.Nov 18 2022, 8:06 AM

rebase and ping

This revision is now accepted and ready to land.Nov 18 2022, 9:41 AM
This revision was automatically updated to reflect the committed changes.

Realized I hadn't included a bailout before ComputeNumSignBitsForTargetNode in this patch. This was unintentional, but there are few enough implementations of this that a quick audit was easy. I didn't see anything concerning. So I'm going to leave this as is instead of inserting the bailout and then removing it again. :)


This can never be true can it?

More relevant to the other exit paths but is return 1; what we want here rather than break;? I asked because the existing scalable vector early exits do the latter (e.g. a couple of lines down) and so I'm wondering if we're loosing information or if the other exit paths are wrong.

reames added inline comments.Nov 18 2022, 12:19 PM

I think this one is dead. Extract_vector_elt should always be returning a vector. We have extract_subvector for vector extract from vector.

As for the 1 vs break thing, both are correct, but 1 is possible non-optimal. The code below the switch uses compute known bits, so if our compute known bits handles one of these and the sign bits are known, then 1 is unnecessarily conservative.

I'll fix both in a follow up commit.

reames added inline comments.Nov 18 2022, 12:30 PM

Addressed in 3fb08d1

reames reopened this revision.Nov 21 2022, 8:28 AM

I reverted this on Friday due to a reported problem with an earlier dependent change. Reopening so that I remember to reland once the underlying issue is resolved. No action needed from reviewers.

Note: When I reland, I will fold in the post-commit review comment and fixes that originally landed separately.

This revision is now accepted and ready to land.Nov 21 2022, 8:28 AM