[PATCH] D24964: TargetLowering: Add SimplifyDemandedBits() helper to TargetLoweringOpt

[Justin Bogner via llvm-commits]

Tom Stellard <thomas.stellard at amd.com> writes:
> tstellarAMD created this revision.
> tstellarAMD added reviewers: bogner, arsenm.
> tstellarAMD added a subscriber: llvm-commits.
> Herald added a subscriber: wdng.
>
> The main purpose of this new helper is to enable simplifying operations that
> have multiple uses.  SimplifyDemandedBits does not handle multiple uses
> currently, and this new function makes it possible to optimize:
>
> and v1, v0, 0xffffff
> mul24 v2, v1, v1      ; Multiply ignoring high 8-bits.
>
> To:
>
> mul24 v2, v0, v0
>
> Where before this would not be optimized, because v1 has multiple uses.

This seems basically reasonable. LGTM after fixing a couple of nitpicks.

> https://reviews.llvm.org/D24964
>
> Files:
>   include/llvm/Target/TargetLowering.h
>   lib/CodeGen/SelectionDAG/TargetLowering.cpp
>
> Index: lib/CodeGen/SelectionDAG/TargetLowering.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/TargetLowering.cpp
> +++ lib/CodeGen/SelectionDAG/TargetLowering.cpp
> @@ -418,6 +418,58 @@
>    return false;
>  }
>  
> +bool
> +TargetLowering::TargetLoweringOpt::SimplifyDemandedBits(SDNode *User,
> +                                                        unsigned OpIdx,
> +                                                        const APInt &Demanded,
> +                                                        DAGCombinerInfo &DCI) {
> +  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
> +  SDValue Op = User->getOperand(OpIdx);
> +  APInt KnownZero, KnownOne;
> +
> +  if (!TLI.SimplifyDemandedBits(Op, Demanded, KnownZero, KnownOne,
> +                                *this, 0, true))

Best to comment the arguments that aren't obvious from context, ie,

  if (!TLI.SimplifyDemandedBits(Op, Demanded, KnownZero, KnownOne,
                                *this, /*Depth=*/0, /*AssumeSingleUse=*/true))

> +    return false;
> +
> +
> +  // Old will not always be the same as Op.  For example:
> +  //
> +  // Demanded = 0xffffff
> +  // Op = i64 truncate (i32 and x, 0xffffff)
> +  // In this case simplify demand bits will want to replace the 'and' node
> +  // with the value 'x', which will give us:
> +  // Old = i32 and x, 0xffffff
> +  // New = x
> +  if (Old.hasOneUse()) {
> +    // For the one use case, we just commit the change.
> +    DCI.CommitTargetLoweringOpt(*this);
> +    return true;
> +  }
> +
> +  // If Old has more than one use then it must be Op, because the
> +  // AssumeSingleUse flag is not propogated to recursive calls of
> +  // SimplifyDemanded bits, so the only node with multiple use that
> +  // it will attempt to combine will be opt.
> +  assert(Old == Op);

Please add a brief message to the assert.

> +
> +  SmallVector <SDValue, 4> NewOps;
> +  for (unsigned i = 0, e = User->getNumOperands(); i != e; ++i) {

LLVM style says these should be called I and E.

> +    if (i == OpIdx) {
> +      NewOps.push_back(New);
> +      continue;
> +    }
> +    NewOps.push_back(User->getOperand(i));
> +  }
> +  DAG.UpdateNodeOperands(User, NewOps);
> +  // Op has less users now, so we may be able to perform additional combines
> +  // with it.
> +  DCI.AddToWorklist(Op.getNode());
> +  // User's operands have been updated, so we may be able to do new combines
> +  // with it.
> +  DCI.AddToWorklist(User);
> +  return true;
> +}
> +
>  /// Look at Op. At this point, we know that only the DemandedMask bits of the
>  /// result of Op are ever used downstream. If we can use this information to
>  /// simplify Op, create a new simplified DAG node and return true, returning the
> @@ -430,7 +482,8 @@
>                                            APInt &KnownZero,
>                                            APInt &KnownOne,
>                                            TargetLoweringOpt &TLO,
> -                                          unsigned Depth) const {
> +                                          unsigned Depth,
> +                                          bool AssumeSingleUse) const {
>    unsigned BitWidth = DemandedMask.getBitWidth();
>    assert(Op.getScalarValueSizeInBits() == BitWidth &&
>           "Mask size mismatches value type size!");
> @@ -442,7 +495,7 @@
>    KnownZero = KnownOne = APInt(BitWidth, 0);
>  
>    // Other users may use these bits.
> -  if (!Op.getNode()->hasOneUse()) {
> +  if (!Op.getNode()->hasOneUse() && !AssumeSingleUse) {
>      if (Depth != 0) {
>        // If not at the root, Just compute the KnownZero/KnownOne bits to
>        // simplify things downstream.
> Index: include/llvm/Target/TargetLowering.h
> ===================================================================
> --- include/llvm/Target/TargetLowering.h
> +++ include/llvm/Target/TargetLowering.h
> @@ -2180,6 +2180,8 @@
>    void operator=(const TargetLowering&) = delete;
>  
>  public:
> +  struct DAGCombinerInfo;
> +
>    /// NOTE: The TargetMachine owns TLOF.
>    explicit TargetLowering(const TargetMachine &TM);
>  
> @@ -2292,6 +2294,16 @@
>      /// generalized for targets with other types of implicit widening casts.
>      bool ShrinkDemandedOp(SDValue Op, unsigned BitWidth, const APInt &Demanded,
>                            const SDLoc &dl);
> +
> +    /// Helper for SimplifyDemandedBits that can simplify an operation with
> +    /// multiple uses.  This function uses TLI.SimplifyDemandedBits to
> +    /// simplify Operand \p OpIdx of \p User and then updated \p User with
> +    /// the simplified version.  No other uses of \p OpIdx are updated.
> +    /// If \p User is the only user of \p OpIdx, this function behaves exactly
> +    /// like TLI.SimplifyDemandedBits except that it also updates the DAG by
> +    /// calling DCI.CommitTargetLoweringOpt.
> +    bool SimplifyDemandedBits(SDNode *User, unsigned OpIdx,
> +                              const APInt &Demanded, DAGCombinerInfo &DCI);
>    };
>  
>    /// Look at Op.  At this point, we know that only the DemandedMask bits of the
> @@ -2301,9 +2313,17 @@
>    /// expression and return a mask of KnownOne and KnownZero bits for the
>    /// expression (used to simplify the caller).  The KnownZero/One bits may only
>    /// be accurate for those bits in the DemandedMask.
> +  /// \p AssumeSingleUse When this paramater is true, this function will
> +  ///    attempt to simplify \p Op even if there are multiple uses.
> +  ///    Callers are responsible for correctly updating the DAG based on the
> +  ///    results of this function, because simply replacing replacing TLO.Old
> +  ///    with TLO.New will be incorrect when this paramater is true and TLO.Old
> +  ///    has multiple uses.
>    bool SimplifyDemandedBits(SDValue Op, const APInt &DemandedMask,
>                              APInt &KnownZero, APInt &KnownOne,
> -                            TargetLoweringOpt &TLO, unsigned Depth = 0) const;
> +                            TargetLoweringOpt &TLO,
> +                            unsigned Depth = 0,
> +                            bool AssumeSingleUse = false) const;
>  
>    /// Determine which of the bits specified in Mask are known to be either zero
>    /// or one and return them in the KnownZero/KnownOne bitsets.