[SandboxIR] Implement BitCastInst

[vporpo]

This patch implements sandboxir::BitCastInst which mirrors llvm::BitCastInst.

[tschuett]

I believe isa + cast is forbidden.

if (Instruction *I = dyn_cast<Instruction>(From))
  return I->getOpcode() == Instruction::Opcode::BitCast;
return false;

[tschuett]

"Note that you should not use an isa<> test followed by a cast<>, for that use the dyn_cast<> operator."

[vporpo]

isa + cast is pretty common for these exact cases where you want to early return if the instruction is of the wrong type. Just grep for it in llvm/lib, you will find lots of examples that look exactly like this.

The alternative would look like:

auto *I = dyn_cast<Instruction>(From);
return I == nullptr ? false : I->getOpcode() == Instruction::Opcode::BitCast;

which is not as elegant and probably slower because of the extra control flow within dyn_cast.

"Note that you should not use an isa<> test followed by a cast<>, for that use the dyn_cast<> operator."

I think this is meant to discourage things like:

  Foo *I = nullptr;
  if (isa<Foo>(V))
     I = cast<Foo>(V);

[aeubanks]

I think both ways will probably optimize to the same code given they're doing the same thing.

can you do return isa<BitCastInst>(From)?

[vporpo]

Do you mean isa<llvm::BitCastInst>(From->Val) ? Yes that should work.

[aeubanks]

but this isn't a multi-instruction? other multi-instructions will have their own implementation of classof that will have to take into account multiple LLVM instructions?

[tschuett]

The LLVM Programmer’s Manual says that you should use dyn_cast. I would follow that.

[vporpo]

It's not, but From could be in a multi-Instruction. And it is theoretically possible to have two separate multi-instructions that contain llvm::BitCastInsts as their Val values. Which is why I think that using From->Val's class is not the right approach. It is safer to get the SandboxIR instruction that points to From->Val and get it's opcode.

[aeubanks]

oh I see, I keep getting confused between LLVM IR and SandboxIR, I haven't spent long enough staring at SandboxIR code to get the namespaces straight in my head

I do slightly prefer tschuett's original suggestion over the current version, it's a bit less redundant even though it's an extra line

[vporpo]

OK I will update it to the dyn_cast one.

[tmsri]

Please use dyn_cast for PtrToInt "classof" check too. I wasn't careful to check the use of isa+cast there.

[vporpo]

I will push an NFC to make this consistent with the rest. Though I still think that the isa + cast pattern looks a bit nicer as it is less verbose.