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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar marked 9 inline comments as done.
aditya_nandakumar added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h:36-38
+  GISelKnownBits() = default;
+  virtual ~GISelKnownBits() = default;
+  void setMF(MachineFunction &MF);
----------------
dsanders wrote:
> 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)
That can certainly work. If I didn't go with the extra unique_ptr, I'd need the default constructor + setMF.
I can update that.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/GISelKnownBits.h:39
+  void setMF(MachineFunction &MF);
+  virtual void computeKnownBitsImpl(Register R, KnownBits &Known,
+                                    const APInt &DemandedElts,
----------------
dsanders wrote:
> Shouldn't this be `protected:` rather than `public:`? Will users be calling this themselves?
This is useful when you're implementing computeKnownBitsForTargetInstr which is in TargetLowering.


================
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);
----------------
dsanders wrote:
> These sound like they're potentially protected/private too
Again - useful for implementing knownBits for target Instrs. For eg, if you have an equivalent instruction to G_FRAME_IDX that's a target PSEUDO, then this allows re-use of existing code.


================
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);
----------------
dsanders wrote:
> Given that there's no implementation for this yet, let's leave it out until we add caching
Will do.


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


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:1-2
+//===- lib/CodeGen/GlobalISel/KnownBitsInfo.cpp ------------------*- C++
+//-*-===//
+//
----------------
dsanders wrote:
> Clang-format broke the header formatting here
Will fix. Thanks for noticing.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65698