[PATCH] D22001: [DSE] Remove dead stores in end blocks containing fence

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Jul-07, at 13:28, Anna Thomas <anna at azul.com> wrote:
> 
> anna marked 2 inline comments as done.
> anna added a comment.
> 
> In http://reviews.llvm.org/D22001#477281, @dexonsmith wrote:
> 
>> I still think this should be the version of `isa<>` that takes a reference.
> 
> 
> Yes, I've changed it to directly take the reference (as done in other dyn_cast checks such as `LoadInst`, `MemTransferInst`  just below the fenceInst check).
> 
>  if (isa<FenceInst>(BBI))

This is not what I think you should do.  This uses an implicit conversion.

I'm advocating for:

    if (isa<FenceInst>(*BBI))

There's no reason to take the address of *BBI.

>        continue;
> 
> It's better to have it as a reference, as the same is done in all other checks for `BBI`.
> 
> The implicit conversion cost as stated in the lang ref:
> Unfortunately, these implicit conversions come at a cost; they prevent these iterators from conforming to standard iterator conventions, and thus from being usable with standard algorithms and containers. For example, they prevent the following code, where B is a BasicBlock, from compiling:
> 
> llvm::SmallVector<llvm::Instruction *, 16>(B->begin(), B->end());
> Because of this, these implicit conversions may be removed some day, and operator* changed to return a pointer instead of a reference.
> 
> 
> http://reviews.llvm.org/D22001
> 
> 
>