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

Wed Apr 20 10:26:00 PDT 2022

fhahn added a comment.

FWIW I am not sure if we should be adding new features to NewGVN at the moment. There are multiple correctness issues of varying degrees. To have a credible path for NewGVN to become useful eventually, I think we should first prioritize fixing the known issues.

================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:3896
+    InstructionsToErase.insert(LoadToOptimize);
+    LoadToOptimize->replaceAllUsesWith(NewValue);
+    LLVM_DEBUG(dbgs() << "Load coersion: The load " << *LoadToOptimize
----------------
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?

================
Comment at: llvm/test/Transforms/NewGVN/load_coercion_between_store_and_load.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -S -gvn < %s | FileCheck %s -check-prefix=GVN-CHECK
+; RUN: opt -S -newgvn < %s | FileCheck %s -check-prefix=NEW-GVN-CHECK

----------------
please keep the RUN line so that the diff shows the functional changes in newgvn. As it is now, it is very hard to see the changes with newgvn

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124071