[PATCH] D124071: [1/3][NewGVN][LoadCoercion] Add support for load coercion between store and load instructions.

[Konstantina Mitropoulou via Phabricator via llvm-commits]

kmitropoulou marked an inline comment as done.
kmitropoulou added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:3896
+    InstructionsToErase.insert(LoadToOptimize);
+    LoadToOptimize->replaceAllUsesWith(NewValue);
+    LLVM_DEBUG(dbgs() << "Load coersion: The load " << *LoadToOptimize
----------------
fhahn wrote:
> I don't think modifying the IR directly here and re-running numbering afterwards really fits with the NewGVN model. If the load is congruent to a known expression, couldn't the load be moved to the same congruence class as that expression and then the existing replace and erase logic will take care of the rest?
Good point! This was my initial approach too. But, I ended up having some problems. NewGVN adds instructions with similar expressions in the same congruence class. In the elimination phase, the algorithm traverses all the congruence classes and it replaces each member of the congruence with the leader of the congruence class.

Let's assume that we have to do load coercion between a vector store and a scalar load. The current algorithm will not add the store and the load in the same congruence class. We have to do a small hack to do this. 

Now, the store and the load are in the same congruence class. In the elimination phase, we can check if any of the members of the congruence class are in LoadCoercion map. If yes, then we can extract the loaded value from the leader of the congruence class (stored value) by generating the right sequence of instructions. This is done at the beginning of the elimination phase. Next, we remove the load from the congruence class because: i. we have to prevent the algorithm from processing this load again and ii. we have to let the algorithm eliminate the other members of the congruence class without a problem. In other words, the load is treated as a special case in the elimination phase. IMO, this does not make much sense since we do not use the elimination phase at all. It is much better to have a separate step that deals with all the loads that need to be optimized with load coercion. After that, we can run the elimination phase without any problem. This is what the current patch does. 

Another problem is that it is not expected to emit instructions during the elimination phase. As a result, it is hard to optimize them. Load coercion emits a new sequence of instructions. For example, in test13, the two loads are replaced by %1 = trunc i32 %V1 to i8. The code has already %V4 = trunc i32 %V1 to i8. The elimination phase will not optimize the two truncates. Because, it traverses the congruence classes in reverse order. So, it first processes the %V4 = trunc i32 %V1 to i8. Next, it processes the congruence class with the two loads that should be optimized with load coercion. It emits the truncate and the value numbering adds the newly generated truncate in the same class as %V4. But, this class has already been processed. As a result, we miss to eliminate %V4 by %1 (or vice versa). To deal with this problem, we have to bookkeep the classes that were updated and next, we should run the elimination phase for these classes again. The current patch does not have this problem, because the newly generated instructions are added in the corresponding congruence classes before the elimination phase.

Another problem is that the elimination phase works without problems if the instructions have correct DFS numbers. If we put the load that needs to be optimized with load coercion in a congruence class, then we have to update the DFS numbers because of the newly generated instructions. This means that we might have to update the DFS numbers several times. This patch updates the DFS numbers only once. 

IMO, this patch is more explicit and more clean since it tries to do the load coercion without complicating the elimination phase and by doing minimal changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124071/new/

https://reviews.llvm.org/D124071