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

Tue Mar 9 14:50:25 PST 2021

nikic 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();
----------------
nagisa wrote:
> 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.
I think @RKSimon's point about the test coverage is less about other targets, and more about the kind of types you test. From a cursory look, the current test changes in this patch are for legal vector types with illegal operations. However, this patch will also make the transform apply to `i57` and `<3 x i17>`, which are untested.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88785