[PATCH] D41672: support phi ranges for machine-level IR

Matthias Braun via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 8 10:50:23 PST 2018


MatzeB added a comment.

LGTM

unittest/CodeGen/MachineInstrTest.cpp looks pretty broken to me already as `createMachineFunction()` constructs things like the Module, LLVMContext and MachineModuleInfo as local variables that really ought to stay alive longer. No idea why this test isn't exploding in asan tests already... That said `phis()` is a convenience function with such an obvious implementation I would be fine without a test (assuming we have a test for MachineBasicBlock::getFirstNonPHI() which of course we haven't; but we could blame someone else for that).

Note/nitpick about coding style: I really don't like `auto` in `for (auto &X : List)` cases. Code is easier to understand for readers if the type of `X` is made explicit as it is not always obvious what a list contains. (Though admittedly I seem to be fighting a loosing battle with my opinion...)


https://reviews.llvm.org/D41672





More information about the llvm-commits mailing list