This is an archive of the discontinued LLVM Phabricator instance.

Separate out BDCE's analysis into a separate DemandedBits analysis.
ClosedPublic

Authored by jmolloy on Jul 29 2015, 8:29 AM.

Details

Reviewers
hfinkel
Summary

This allows other areas of the compiler (LoopVectorize, soon!) to use BDCE's bit-tracking.

NFCI.

Diff Detail

Repository
rL LLVM

Event Timeline

jmolloy updated this revision to Diff 30909.Jul 29 2015, 8:29 AM
jmolloy retitled this revision from to Separate out BDCE's analysis into a separate DemandedBits analysis..
jmolloy updated this object.
jmolloy added a reviewer: hfinkel.
jmolloy set the repository for this revision to rL LLVM.
jmolloy added a subscriber: llvm-commits.

Ping! Hal, do you have any comments on this?

hfinkel added inline comments.Aug 10 2015, 1:11 AM
include/llvm/Analysis/DemandedBits.h
55

I think that I prefer the present tense for analysis results, let's name this:

isInstructionDead
lib/Analysis/DemandedBits.cpp
352

Given that you return a conservative answer regardless, I don't see any reason to have this assert. I recommend removing it (and the associated comment in the header).

355

Use getSizeInBits instead of getScalarSizeInBits so that we get the right conservative answer for vectors.

lib/Transforms/Scalar/BDCE.cpp
82

Is looks like, as you have it setup, getDemandedBits will return all ones for an instruction not in the map (for any instruction for which DB.demandedBitsKnown(&I) would return false). As a result, we don't need the DB.demandedBitsKnown(&I) check and can just use the DB.getDemandedBits check (avoiding repeating the map lookup).

jmolloy updated this revision to Diff 31950.Aug 12 2015, 9:43 AM

Hi Hal,

I've updated the diff with most of your comments updated, however:

Is looks like, as you have it setup, getDemandedBits will return all ones for an instruction not in the map (for any instruction for which DB.demandedBitsKnown(&I) would return false). As a result, we don't need the DB.demandedBitsKnown(&I) check and can just use the DB.getDemandedBits check (avoiding repeating the map lookup).

Unfortunately this isn't the case. BDCE expects to handle the case where an instruction was analyzed, but has bits demanded differently to an instruction that wasn't even analyzed. In the second case it expects to dive off down the else path, in the first case it doesn't. Changing that behavior at all leads to lots of use-before-free asserts.

James

hfinkel edited edge metadata.Aug 12 2015, 10:33 AM

Hi Hal,

I've updated the diff with most of your comments updated, however:

Is looks like, as you have it setup, getDemandedBits will return all ones for an instruction not in the map (for any instruction for which DB.demandedBitsKnown(&I) would return false). As a result, we don't need the DB.demandedBitsKnown(&I) check and can just use the DB.getDemandedBits check (avoiding repeating the map lookup).

Unfortunately this isn't the case. BDCE expects to handle the case where an instruction was analyzed, but has bits demanded differently to an instruction that wasn't even analyzed. In the second case it expects to dive off down the else path, in the first case it doesn't. Changing that behavior at all leads to lots of use-before-free asserts.

I'm not sure what exactly you did, but the underlying logic should be this:

if (instruction is dead) {
  mark for deletion
} else if (all bits are dead and has an integer type) {
  replace all uses with zero
  mark for deletion 
}

if (marked for deletion && not an always-live instruction) {
  drop references and add to the to-delete worklist
}

I believe to implement my suggestion, you'll need to slightly rearrange the logic there. Thanks again!

James

jmolloy updated this revision to Diff 32039.Aug 13 2015, 2:56 AM
jmolloy edited edge metadata.

Hi Hal,

I worked out where I was getting confused. In DemandedBits, the "Visited" set only contains non-integer instructions. In "isInstructionDead" I was only visiting membership of the Visited set - I needed to check membership of AliveBits too because that's where all the integer types live.

With that change, I've been able to do away with a no-longer needed API function and the code looks a lot neater.

Cheers,

James

hfinkel accepted this revision.Aug 13 2015, 5:31 PM
hfinkel edited edge metadata.

One comment below, otherwise LGTM.

lib/Transforms/Scalar/BDCE.cpp
94

You don't need this check here because you also have the isAlwaysLive check in DB.isInstructionDead (and I think that's a good thing).

This revision is now accepted and ready to land.Aug 13 2015, 5:31 PM
jmolloy closed this revision.Aug 14 2015, 4:10 AM

Hi Hal,

Thanks, I removed the isAlwaysLive() check (and function) and simplified the logic in r245038.

Cheers,

James