[PATCH] D121744: [SCCP] Update ValueLatticeElement blockaddresses when removing unreachable BasicBlocks

Tue Mar 15 15:08:11 PDT 2022

nickdesaulniers added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:528-531
+        LLVMContext &Context = BB->getParent()->getContext();
+        ConstantInt *CI = ConstantInt::get(Type::getInt32Ty(Context), 1);
+        Constant *C =
+            ConstantExpr::getIntToPtr(CI, Type::getInt8PtrTy(Context));
----------------
There's probably some code somewhere in `llvm::SimplifyInstruction` that already does this transform (as observed by `SimplifyCFG` in https://github.com/llvm/llvm-project/issues/54328#issuecomment-1067229660). maybe that can be reused here?

Since the behavior is surprising to users (https://github.com/llvm/llvm-project/issues/54328) maybe we could consolidate such transforms with a comment about why this is done, for historical context?

================
Comment at: llvm/lib/Transforms/Utils/SCCPSolver.cpp:424-426
+        if (New->getType()->isPointerTy() && Old->getType()->isPointerTy() &&
+            New->getType() != Old->getType())
+          New = ConstantExpr::getBitCast(New, Old->getType());
----------------
This added bitcast is a bit curious, I'd like to clean it up, but I'm not sure yet how best to do so.

Basically, we know we want to replace `i8* blockaddress(@fn, %bb)` with `i8* inttoptr (i32 1 to i8*)`, but the `User` of the BA and entry in `ValueState` is `%1 = bitcast i8* blockaddress(@fn, %bb) to i64*`.

Maybe I should be doing an operand replacement? As in take `Old` and replace its operand with `New`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121744