[PATCH] D87976: Support the division-by-constant strength reduction for more integer types

[Simonas Kazlauskas via Phabricator via llvm-commits]

nagisa added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4954
   EVT SVT = VT.getScalarType();
-  EVT ShVT = getShiftAmountTy(VT, DAG.getDataLayout());
+  EVT ShVT = getShiftAmountTy(VT, DAG.getDataLayout(), IsAfterLegalization);
   EVT ShSVT = ShVT.getScalarType();
----------------
craig.topper wrote:
> IsAfterLegalization refers to LegalOperations not LegalTypes.
Any suggestions on how to best approach obtaining the information? Should I just pass in another boolean as an argument from `DAGCombiner`?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4959
   // Check to see if we can do this.
-  // FIXME: We should be more aggressive here.
-  if (!isTypeLegal(VT))
+  if (IsAfterLegalization && !isTypeLegal(VT))
     return SDValue();
----------------
craig.topper wrote:
> I believe IsAfterLegalization refers to LegalOperations rather than LegalTypes.
> 
> But if we get here after type legalization then the VT must be Legal or it wouldn't have been seen by DAGCombiner to call this so we might just be able to remove this whole check.
Yeah, I had this removed entirely in an earlier revision and it worked just fine, I had this added back motivating to myself that there might be //some// weird corner-case that I'm not aware of where “just do it” approach wouldn't be correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87976