[llvm-commits] Review Request for modification to Instruction::IsIdenticalToWhenDefined

[Duncan Sands]

Hi Joel,

> --- lib/VMCore/Instruction.cpp	(revision 156460)
> +++ lib/VMCore/Instruction.cpp	(working copy)
> @@ -226,7 +226,15 @@
>              RMWI->isVolatile() == cast<AtomicRMWInst>(I)->isVolatile()&&
>              RMWI->getOrdering() == cast<AtomicRMWInst>(I)->getOrdering()&&
>              RMWI->getSynchScope() == cast<AtomicRMWInst>(I)->getSynchScope();
> -
> +	if (const PHINode *thisPHI = dyn_cast<PHINode>(this)) {

this is not indented consistently with the other "if" statements above.

> +		// The check above ensures that the dyn_cast and getIncomingBlock calls
> +		// below won't fail, as the type of otherPHI must be PHINode and the
> +		// number of incoming blocks must be the same.

I think this comment is pointless, as this is obvious and the analogous
situation occurs for all the other kinds of instructions just above.

> +		const PHINode *otherPHI = dyn_cast<PHINode>(I);

This should be cast, not dyn_cast, since you know it must succeed.  This is
also indented wrong (should be by two spaces).  Indentation is also funky
below but I won't mention it again.

> +		for (unsigned i = 0, e = thisPHI->getNumOperands(); i != e; ++i)
> +		  if (thisPHI->getIncomingBlock(i) != otherPHI->getIncomingBlock(i))
> +			  return false;

If you get through the loop, I think you should have an explicit "return true;"
here.  I know that right now you fall through to the "return true" below, but
if some more instruction types are added later (eg SwitchInst) then by falling
through you risk doing a bunch of extra pointless testing.

> +	}
>     return true;
>   }

I mention SwitchInst because while it is OK right now, there is a plan to change
its implementation, which will probably mean it will need to be added here too.

Ciao, Duncan.