[PATCH] [SimplifyCFG] threshold for folding branches with common destination

[Hal Finkel]

----- Original Message -----
> From: "Jingyue Wu" <jingyue at google.com>
> To: jingyue at google.com, nrotem at apple.com, eliben at google.com, meheff at google.com, resistor at mac.com
> Cc: hfinkel at anl.gov, llvm-commits at cs.uiuc.edu
> Sent: Monday, September 29, 2014 6:43:08 PM
> Subject: Re: [PATCH] [SimplifyCFG] threshold for folding branches with common destination
> 
> ================
> Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:2034
> @@ +2033,3 @@
> +      continue;
> +    if (!I->hasOneUse() || !isSafeToSpeculativelyExecute(I, DL))
> +      return false;
> ----------------
> hfinkel wrote:
> > I don't think that the hasOneUse check here really does what you
> > want once we allow for more than once instruction. We used to
> > check that the single bonus instruction had one user and this user
> > specifically was the Cond. Now you'd like to allow for some
> > variable number of single-use instructions (regardless of a
> > relationship to Cond). This could change behavior even when
> > allowing only a single instruction.
> > 
> > I think that what you really want to do is to walk up the operand
> > graph from Cond, accumulating instructions until you reach your
> > limit, keeping the hasOneUse check and the check that the use is
> > Cond for the first instruction.
> Hi Hal,
> 
> Thanks for the careful review! However, I don't think the modified
> code changes the behavior when allowing only a single bonus
> instruction. Note that in Line 2037 I check whether the only user is
> in the same BB (and appears after the potential bonus instruction
> otherwise def doesn't dominate use). When there is only one
> potential bonus instruction, it is either used by Cond or BI
> (DbgInfoIntrinsic only uses MDNode but not Instruction). Being used
> by BI is impossible, because BI only uses Cond as its first operand
> and other operands are all BB labels. Therefore, this bonus
> instruction must be used by Cond. Does this make sense? I agree it
> is at least worth a comment.

Yes, I understand now, thanks! Please do add a comment.

 -Hal

> 
> I like your suggestion of early exiting once we reach the limit. I'll
> change that part.
> 
> Thanks,
> Jingyue
> 
> http://reviews.llvm.org/D5529
> 
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory