[PATCH] D51596: Fixed bug in MachineInstr::hasPropertyInBundle

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

It seems reasonable to match the type of llvm::MCInstrDesc::getFlags() which is uint64_t nowadays. The hasPropertyInBundle method is private to MachineInstr, and afaict only used from MachineInstr::hasProperty. This is kind of a no-brainer to me, and I think we do not need a regression test here.

Although, one idea is to add an assert that MCFlag<64 in MachineInstr::hasProperty.
Another idea could be to add an assert that "Mask != 0" in MachineInstr::hasPropertyInBundle. 
And maybe we should have a static (compile time) assert somewhere that the highest value in MCID::Flag is less than 64.

So I think this patch looks good as-is. But feel free to for example add the assert on MCFlag<64 in MachineInstr::hasProperty. I assume that could have helped in detecting this fault?

A small NIT on the first line in the commit message is that it usually is written in "imperative mood" (see https://chris.beams.io/posts/git-commit/#imperative). I don't say that it is mentioned in the dev policy (https://llvm.org/docs/DeveloperPolicy.html#commit-messages) but I think it is common practice.


Repository:
  rL LLVM

https://reviews.llvm.org/D51596