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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with some nits fixed



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h:36-38
+  GISelKnownBits() = default;
+  virtual ~GISelKnownBits() = default;
+  void setMF(MachineFunction &MF);
----------------
Do you still need the default-constructor+setMF() set up? I think we could move all the initialization to the constructor and have GISelKnownBitsAnalysis::get() take the MF and allocate/re-use a std::unique_ptr<GISelKnownBits>

What I'm thinking long term when we add caching is that the pass would have an instance of GISelKnownBits per-function and would allocate them lazily as get(MF) is called. Flushing the whole cache is then as simple as freeing the GISelKnownBits and re-constructing it in the next get(MF)


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h:39
+  void setMF(MachineFunction &MF);
+  virtual void computeKnownBitsImpl(Register R, KnownBits &Known,
+                                    const APInt &DemandedElts,
----------------
Shouldn't this be `protected:` rather than `public:`? Will users be calling this themselves?


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h:50-60
+  // FIXME: Is this the right place for G_FRAME_INDEX? Should it be in
+  // TargetLowering?
+  void computeKnownBitsForFrameIndex(Register R, KnownBits &Known,
+                                     const APInt &DemandedElts,
+                                     unsigned Depth = 0);
+  static unsigned inferAlignmentForFrameIdx(int FrameIdx, int Offset,
+                                            const MachineFunction &MF);
----------------
These sound like they're potentially protected/private too


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h:62-64
+  // Manually set KnownBits for a register.
+  // Only used if caching/serializing to MIR etc.
+  void setKnownBits(Register R, const KnownBits &Known);
----------------
Given that there's no implementation for this yet, let's leave it out until we add caching


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h:79
+
+  constexpr unsigned getMaxDepth() const { return 6; }
+};
----------------
I believe this should be protected/private too


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:1-2
+//===- lib/CodeGen/GlobalISel/KnownBitsInfo.cpp ------------------*- C++
+//-*-===//
+//
----------------
Clang-format broke the header formatting here


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65698