Martien.kissfix #185
Martien.kissfix #185
Conversation
martien-de-jong
requested review from
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
khallouh,
konstantinschwarz,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| // Ensure that next MI is valid | ||
| auto NextMI = std::next(MachineBasicBlock::instr_iterator(MI)); | ||
| if (NextMI == MBB.instr_end() || NextMI == LastPHI) | ||
| const auto End = MBB.end(); |
There was a problem hiding this comment.
We are comparing every pointer PHI node in the block to any other pointer PHI node in that block. We know that all PHI nodes are consecutive at the start of the block. The transformation only changes operands in other instructions, so iterators stay intact. By writing the loops using basic iterator idiom reveals the symmetry between the two loops. It also avoids using constructs like .phis() and .getFirstNonPhi() that both perform exactly the same iteration as the main loops.
| break; | ||
|
|
||
| for (auto &PHI : make_early_inc_range(make_range( | ||
| NextMI, MachineBasicBlock::instr_iterator(*LastPHI)))) { |
There was a problem hiding this comment.
The error was here. LastPHI was at .end(), which means you cannot dereference it to create an instr_iterator. Note that in that case the validity check reads if (NextMI == end || NextMI == end)
| @@ -120,26 +120,26 @@ class AIEEliminateDuplicatePHI : public MachineFunctionPass { | |||
| bool processMBB(MachineBasicBlock &MBB, MachineRegisterInfo &MRI, | |||
| MachineIRBuilder &MIB, GISelObserverWrapper &Observer) { | |||
| bool Changed = false; | |||
| auto LastPHI = MBB.getFirstNonPHI(); | |||
There was a problem hiding this comment.
I would normally expect an iterator with this name to point to the last phi, not the the first non-phi.
|
|
||
| bb.3: | ||
| %3:_(p0) = G_PHI %2:_(p0), %bb.1, %3:_(p0), %bb.2 | ||
| %4:_(p0) = G_PHI %2:_(p0), %bb.1, %3:_(p0), %bb.2 |
There was a problem hiding this comment.
This is just triggering the crash. These are equivalent phis, and the test can be strengthened by using %4 in a downstream block.
martien-de-jong
force-pushed
the
martien.kissfix
branch
from
ec96e3c to
dfbae40
Compare
| if (!isPointerTypePHI(MRI, PhiA)) | ||
| continue; | ||
| auto PhiItB = PhiItA; | ||
| for (++PhiItB++; PhiItB != End; ++PhiItB) { |
There was a problem hiding this comment.
Looks interesting: ++PhiItB++
martien-de-jong
force-pushed
the
martien.kissfix
branch
from
dfbae40 to
5c1a88b
Compare
|
I approved the competing PR |
dup phi elimination crashed in a supertest on a block with just two phi nodes