Page MenuHomePhabricator

support phi ranges for machine-level IR
ClosedPublic

Authored by bob.wilson on Jan 2 2018, 11:16 AM.

Details

Summary

Add iterator ranges for machine instruction phis, similar to the IR-level
phi ranges added in r303964. I updated a few places to use this. Besides
general code simplification, this change will allow removing a non-upstream
change from Swift's copy of LLVM (in a better way than my previous attempt
in http://reviews.llvm.org/D19080).

Diff Detail

Event Timeline

bob.wilson created this revision.Jan 2 2018, 11:16 AM
bkramer accepted this revision.Jan 2 2018, 1:02 PM

lgtm

This revision is now accepted and ready to land.Jan 2 2018, 1:02 PM
davide accepted this revision.Jan 2 2018, 2:58 PM
davide added a subscriber: davide.

Nice to see this for MBB, +1 modulo two minors.

CodeGen/MachineInstrTest.cpp
262–265

EXPECT in gtest is non-fatal so maybe you want ASSERT_ here
https://github.com/google/googletest/blob/master/googletest/docs/Primer.md

llvm/CodeGen/MachineBasicBlock.h
228–233

Doxygen comment for these two?

bob.wilson closed this revision.Jan 3 2018, 6:59 PM

OK. Committed with Davide's suggested changes in r321783.

Unfortunately that unit test doesn't work. I didn't notice the problem but the sanitizer bot exposed it. The existing bogus target in MachineInstrTest.cpp is only good enough to create instructions but not sufficient to insert them into basic blocks. The addNodeToList ilist callback dereferences the pointer to the MachineRegisterInfo. Adding MachineRegisterInfo would also require TargetRegisterInfo, and even a minimal implementation of that would be quite complicated. If anyone has a good idea of an alternative, please let me know. In the meantime, I have removed the unit test in r321784.

Gerolf added a subscriber: Gerolf.

+ Matthias for review & thoughts about the unit test.

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...)