From f98032ef64a777fe63d390539f901d7d6d4e868c Mon Sep 17 00:00:00 2001 From: Bob Wilson Date: Mon, 12 Oct 2009 21:23:15 +0000 Subject: [PATCH] Last week, ARMConstantIslandPass was failing to converge for the MultiSource/Benchmarks/MiBench/automotive-susan test. The failure has since been masked by an unrelated change (just randomly), so I don't have a testcase for this now. Radar 7291928. The situation where this happened is that a constant pool entry (CPE) was placed at a lower address than the load that referenced it. There were in fact 2 CPEs placed at adjacent addresses and referenced by 2 loads that were close together in the code. The distance from the loads to the CPEs was right at the limit of what they could handle, so that only one of the CPEs could be placed within range. On every iteration, the first CPE was found to be out of range, causing a new CPE to be inserted. The second CPE had been in range but the newly inserted entry pushed it too far away. Thus the second CPE was also replaced by a new entry, which in turn pushed the first CPE out of range. Etc. Judging from some comments in the code, the initial implementation of this pass did not support CPEs placed _before_ their references. In the case where the CPE is placed at a higher address, the key to making the algorithm terminate is that new CPEs are only inserted at the end of a group of adjacent CPEs. This is implemented by removing a basic block from the "WaterList" once it has been used, and then adding the newly inserted CPE block to the list so that the next insertion will come after it. This avoids the ping-pong effect where CPEs are repeatedly moved to the beginning of a group of adjacent CPEs. This does not work when going backwards, however, because the entries at the end of an adjacent group of CPEs are closer than the CPEs earlier in the group. To make this pass terminate, we need to maintain a property that changes can only happen in some sort of monotonic fashion. The fix used here is to require that the CPE for a particular constant pool load can only move to lower addresses. This is a very simple change to the code and should not cause any significant degradation in the results. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@83902 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Target/ARM/ARMConstantIslandPass.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/Target/ARM/ARMConstantIslandPass.cpp b/lib/Target/ARM/ARMConstantIslandPass.cpp index e47ddd18a54..36eeffce269 100644 --- a/lib/Target/ARM/ARMConstantIslandPass.cpp +++ b/lib/Target/ARM/ARMConstantIslandPass.cpp @@ -946,10 +946,11 @@ MachineBasicBlock *ARMConstantIslands::AcceptWater(water_iterator IP) { /// LookForWater - look for an existing entry in the WaterList in which /// we can place the CPE referenced from U so it's within range of U's MI. /// Returns true if found, false if not. If it returns true, NewMBB -/// is set to the WaterList entry. -/// For ARM, we prefer the water that's farthest away. For Thumb, prefer -/// water that will not introduce padding to water that will; within each -/// group, prefer the water that's farthest away. +/// is set to the WaterList entry. For Thumb, prefer water that will not +/// introduce padding to water that will. To ensure that this pass +/// terminates, the CPE location for a particular CPUser is only allowed to +/// move to a lower address, so search backward from the end of the list and +/// prefer the first water that is in range. bool ARMConstantIslands::LookForWater(CPUser &U, unsigned UserOffset, MachineBasicBlock *&NewMBB) { if (WaterList.empty()) @@ -960,7 +961,9 @@ bool ARMConstantIslands::LookForWater(CPUser &U, unsigned UserOffset, for (water_iterator IP = prior(WaterList.end()), B = WaterList.begin();; --IP) { MachineBasicBlock* WaterBB = *IP; - if (WaterIsInRange(UserOffset, WaterBB, U)) { + // Check if water is in range and at a lower address than the current one. + if (WaterIsInRange(UserOffset, WaterBB, U) && + WaterBB->getNumber() < U.CPEMI->getParent()->getNumber()) { unsigned WBBId = WaterBB->getNumber(); if (isThumb && (BBOffsets[WBBId] + BBSizes[WBBId])%4 != 0) { @@ -1109,10 +1112,7 @@ bool ARMConstantIslands::HandleConstantPoolUser(MachineFunction &MF, // We will be generating a new clone. Get a UID for it. unsigned ID = AFI->createConstPoolEntryUId(); - // Look for water where we can place this CPE. We look for the farthest one - // away that will work. Forward references only for now (although later - // we might find some that are backwards). - + // Look for water where we can place this CPE. if (!LookForWater(U, UserOffset, NewMBB)) { // No water found. DEBUG(errs() << "No water found\n");