[PATCH] D65698: [GISel]: Add GISelKnownBits analysis

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h:81
+protected:
+  virtual unsigned getMaxDepth() { return 6; }
+};
----------------
I would expect this to be a pass parameter, not a virtual function 


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:28
+
+INITIALIZE_PASS_BEGIN(GISelKnownBitsPass, DEBUG_TYPE,
+                      "Analysis for ComputingKnownBits", false, true)
----------------
Remove Pass to fix PassPass


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:100
+
+  if (!DstTy.isScalar() && MI.getOpcode() != TargetOpcode::G_FRAME_INDEX)
+    return; // TODO: Handle vectors and other opcodes.
----------------
Should probably just be !isVector and allow all pointers


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:258
+    if (MI.hasOneMemOperand())
+      Known.Zero.setBitsFrom((*MI.memoperands_begin())->getSize() * 8);
+    break;
----------------
I vaguely remember a getSizeInBits being added to MMO? If not it should be


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:274
+    uint64_t Shift = RHSKnown.getConstant().getZExtValue();
+    LLVM_DEBUG(dbgs() << "[" << Depth << "] Shift is " << Shift << "\n");
+
----------------
Single quotes around single characters 


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:299
+  case TargetOpcode::G_PTRTOINT:
+    // Fall through and handle them the same as zext/trunc.
+    LLVM_FALLTHROUGH;
----------------
I thought these disallowed mismatched sizes


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:319
+  assert(!Known.hasConflict() && "Bits known to be one AND zero?");
+  LLVM_DEBUG(dbgs() << "[" << Depth << "] Compute known bits: " << MI);
+  LLVM_DEBUG(dbgs() << "[" << Depth << "] Computed for: " << MI);
----------------
Merge consecutive LLVM_DEBUG into one


================
Comment at: llvm/unittests/CodeGen/GlobalISel/KnownBitsTest.cpp:25
+  KnownBits Res = Info.getKnownBits(SrcReg);
+  EXPECT_EQ(Res.One.getZExtValue(), (uint64_t)1);
+  EXPECT_EQ(Res.Zero.getZExtValue(), (uint64_t)0xfe);
----------------
Gtest defines the order of these the opposite way, so these should be swapped for the correct failure message 


================
Comment at: llvm/unittests/CodeGen/GlobalISel/KnownBitsTest.cpp:51
+  KnownBits Known = Info.getKnownBits(SrcReg);
+  ASSERT_FALSE(Known.hasConflict());
+  EXPECT_EQ(Known.One.getZExtValue(), 0u);
----------------
EXPECT instead of ASSERT?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65698