[PATCH] D98378: [LSR] RFC: Fix crash in Phi node with EHPad block

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:3452
+        // Check if a PHI node has reference-equal inputs which are terminated
+        // with non-splittable instructions
+        if (isa<PHINode>(UserInst)) {
----------------
Period at the end of comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:3457
+          llvm::Value *ExpectedValue = U;
+          for (unsigned int I = 0; I < PhiNode->getNumIncomingValues(); I++) {
+            if (PhiNode->getIncomingValue(I) == ExpectedValue) {
----------------
Instead of iterating through all the incoming values, can we use `Use::getOperandNo` to only check the relevant one?


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/X86/issue48708.ll:1
+; RUN: opt -loop-reduce -S %s
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:6:2:64-S128"
----------------
Could you add check lines to make sure we do something sensible?
No crashing doesn't mean we are correct :).


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/X86/issue48708.ll:2
+; RUN: opt -loop-reduce -S %s
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:6:2:64-S128"
+
----------------
Could you add a comment on what this test is checking and what we should/shouldn't observe here?
In particular putting the PR id here is a good idea.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/X86/issue48708.ll:3
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:6:2:64-S128"
+
+%"class.absl::Storage" = type {}
----------------
Could you change the name of the test case to something more descriptive?


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/X86/issue48708.ll:11
+define void @0(%"class.absl::Storage"* %0, %"struct.absl::allocator_traits" %1) personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+  %3 = alloca %"struct.absl::StorageView", align 8
+  %4 = alloca i8, align 1
----------------
Could you get rid of the implicit variables (%[0-9]+)?
You can use `opt -instnamer -S` for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98378