[PATCH] D69116: [TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (1/2)

[Joan LLuch via Phabricator via llvm-commits]

joanlluch added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2614
+  virtual unsigned getShiftAmountThreshold(EVT VT) const {
+    return VT.getSizeInBits();
+  }
----------------
spatel wrote:
> It may not be used with vector types currently, but it would be safer to use:
>   VT.getScalarSizeInBits()
okey


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3622
+          if (AndRHS->getAPIntValue().isPowerOf2() &&
+                ShCt <= TLI.getShiftAmountThreshold(ShiftTy)) {
             return DAG.getNode(ISD::TRUNCATE, dl, VT,
----------------
spatel wrote:
> This is probably not working as you expected because the shift type may not be the same as the value type.
> I added an x86 test to demonstrate:
> rL375158
Would passing the shift value  (not the constant amount type) to getShiftAmountThreshold work in all cases?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:3633-3634
             return DAG.getNode(ISD::TRUNCATE, dl, VT,
                                DAG.getNode(ISD::SRL, dl, N0.getValueType(), N0,
-                                      DAG.getConstant(C1.logBase2(), dl,
-                                                      ShiftTy)));
+                                      DAG.getConstant(ShCt, dl, ShiftTy)));
           }
----------------
spatel wrote:
> Here and above the indentation looks non-standard. Use "clang-format" to correct.
I didn't know about clang-format. I see what you mean now. I will correct that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69116