This is an archive of the discontinued LLVM Phabricator instance.

Add getDemandedBits for uses.
ClosedPublic

Authored by qunyanm on Feb 19 2021, 11:51 AM.

Details

Summary

Add getDemandedBits method for uses so we can query demanded bits for each use. This can help getting better use information. For example, for the code below
define i32 @test_use(i32 %a) {

%1 = and i32 %a, -256
%2 = or i32 %1, 1
%3 = trunc i32 %2 to i8 (didn't optimize this to 1 for illustration purpose)
... some use of %3
ret %2

}
if we look at the demanded bit of %2 (which is all 32 bits because of the return), we would conclude that %a is used regardless of how its return is used. However, if we look at each use separately, we will see that the demanded bit of %2 in trunc only uses the lower 8 bits of %a which is redefined, therefore %a's usage depends on how the function return is used.

Diff Detail

Event Timeline

qunyanm created this revision.Feb 19 2021, 11:51 AM
qunyanm requested review of this revision.Feb 19 2021, 11:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 11:51 AM
qunyanm changed the visibility from "Public (No Login Required)" to "No One".Feb 19 2021, 12:09 PM
qunyanm changed the edit policy from "All Users" to "No One".
qunyanm removed subscribers: llvm-commits, hiraditya.
qunyanm updated this revision to Diff 325456.Feb 22 2021, 8:30 AM
  • Formatting.
qunyanm changed the visibility from "No One" to "Public (No Login Required)".
qunyanm changed the edit policy from "No One" to "All Users".
qunyanm added a subscriber: llvm-commits.

The patch's description states what the patch does, but not *why* it does that.

qunyanm edited the summary of this revision. (Show Details)Feb 22 2021, 9:59 AM
qunyanm edited the summary of this revision. (Show Details)
qunyanm added a subscriber: test.
qunyanm removed a subscriber: test.Feb 22 2021, 10:14 AM

The patch's description states what the patch does, but not *why* it does that.

+1

llvm/lib/Analysis/DemandedBits.cpp
465

Could this be used in the APInt::getAllOnesValue() above instead of the getTypeSizeInBits call?

qunyanm updated this revision to Diff 330843.Mar 15 2021, 4:49 PM
qunyanm edited the summary of this revision. (Show Details)
  • Use getScalarSizeInBits() to get bitwidth of the type.
qunyanm updated this revision to Diff 330851.Mar 15 2021, 5:16 PM
qunyanm marked an inline comment as done.
  • Formatting.
  • Use getScalarSizeInBits() to get bitwidth of the type.

The patch's description states what the patch does, but not *why* it does that.

I reworded the commit message. Just realized that I'm not sure if that's the patch's description you were referring to. Please let me know if it's wrong and how to update it. Sorry, I should've asked first.

llvm/lib/Analysis/DemandedBits.cpp
465

Yes. Thanks!

I think we'll need to see the patch that depends on this.

qunyanm updated this revision to Diff 331109.Mar 16 2021, 2:32 PM

Revert last commit where getTypeSizeInBit() is replaced by getScalarSizeInBits() because
getScalareSizeInBits() returns 0 for cases such as non-primitive types where the use should
be all bit used.

RKSimon added inline comments.Mar 16 2021, 2:50 PM
llvm/lib/Analysis/DemandedBits.cpp
465

Hmm - what about

unsigned BitWidth = DL.getTypeSizeInBits(T->getScalarType());

I think we'll need to see the patch that depends on this.

I don't have a patch to submit that depends on this (I just use it in a tool built on top of llvm). The change is basically a wrapper to expose determineLiveOperandBits(), which is private, and set up the dummy arguments. It would allow inspection of demandedBits for each use in case we want finer grain information than the demandedBits of def which is an aggregate of all its uses..

llvm/lib/Analysis/DemandedBits.cpp
465

Actually it won't work since getScalarSizeInBits() returns 0 for non-primitive types that demandedBits don't track therefore should be marked as all bit used.

qunyanm updated this revision to Diff 331122.Mar 16 2021, 3:59 PM
  • Replace getScalarSizeInBits() with DL.getTypeSizeInBits since they return the same value for int.
qunyanm added inline comments.Mar 16 2021, 4:03 PM
llvm/lib/Analysis/DemandedBits.cpp
465

Yes, I believe they return the same value for int/intvector.

I've no strong objections to this going in purely for debug purposes, but would prefer it if there were use cases in the actual code.

RKSimon requested changes to this revision.May 4 2021, 7:11 AM

This either needs use cases in code, or needs cleaning up to just operate in debug mode.

This revision now requires changes to proceed.May 4 2021, 7:11 AM

We have an out-of-tree code analysis tool that we want to surface use-level consumed-bits information, and thus want this method to be added to the public API surface of libLLVM (and therefore need it to be included in release builds). Part of the analysis includes propagating the demanded bits of returned values. So for the code example from the commit message:

define i32 @test_use(i32 %a) {
      %1 = and i32 %a, -256
      %2 = or i32 %1, 1
      %3 = trunc i32 %2 to i8 (didn't optimize this to 1 for illustration purpose)
      ... some use of %3
      ret %2
  }

In order to propagate the demanded bits of the returned value, we need to be able to get the demanded bits of %2 at "trunc". This change is just trying to make "determineLiveOperandBits" public as was done for "determineLiveOperandBitsAdd/determineLiveOperandBitsSub", and I added the use in the dump to make sure that the lit test suite exercises the code and verifies the result. Could you please clarify the concern/request? I.e., if the dump code executed in the lit test is not "actual code", but our motivating use case is an out-of-tree llvm-based code analysis tool, what code do you recommend I add a use in?

craig.topper added inline comments.
llvm/lib/Analysis/DemandedBits.cpp
461

Please fix this clang-format warning

462

Drop curly braces

qunyanm updated this revision to Diff 344954.May 12 2021, 2:02 PM
  • Formatting.
qunyanm marked 2 inline comments as done.May 12 2021, 2:05 PM
RKSimon accepted this revision.May 27 2021, 2:45 AM

OK, let's go with this - LGTM (after you fix that clang-tidy warning)

llvm/lib/Analysis/DemandedBits.cpp
516

auto PrintDB

This revision is now accepted and ready to land.May 27 2021, 2:45 AM
qunyanm updated this revision to Diff 348277.May 27 2021, 8:22 AM
  • Formatting
qunyanm marked an inline comment as done.May 27 2021, 8:23 AM
This revision was landed with ongoing or failed builds.Jun 2 2021, 7:08 AM
Closed by commit rGcbde2487367a: Add getDemandedBits for uses. (authored by qunyanm, committed by JosephTremoulet). · Explain Why
This revision was automatically updated to reflect the committed changes.