[PATCH] D88785: Support {S,U}REMEqFold before legalization

[Simonas Kazlauskas via Phabricator via llvm-commits]

nagisa added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5456
   // If MUL is unavailable, we cannot proceed in any case.
-  if (!isOperationLegalOrCustom(ISD::MUL, VT))
+  if (!DCI.isBeforeLegalizeOps() && !isOperationLegalOrCustom(ISD::MUL, VT))
     return SDValue();
----------------
RKSimon wrote:
> Doesn't this mean we're opening this to work on illegal types? At the very least we'd need decent test coverage for that case.
The idea here was that its better to apply this fold, regardless of whether the simple types/operations are legal or not, and then let the type/operation legalization take care of legalizing the strength reduced graph, instead.

This to me makes a lot of sense, because as far as I can tell, making the `ISD::MUL`, `ISD::SUB`, `ISD::ADD` & `ISD::ROTR` operations legal ought to be significantly easier (and result in simpler code) than making a `ISD::{U,S}REM` legal. Furthermore, if these simpler operations are not legal, it is most likely the case that the `ISD::{U,S}REM` itself won't be either and legalization of some sort will have to occur regardless.

I see what you're saying about the test coverage – the current test suite only really covers Aarch64 and x86. I will look into porting these test cases to more of the other targets.

Thank you for the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88785