[PATCH] D153620: [X86] Combine MUL+SRL+TRUNC to MULX for i32 on 64-bit

Sat Jun 24 05:30:43 PDT 2023

pengfei added a comment.

In D153620#4445950 <https://reviews.llvm.org/D153620#4445950>, @craig.topper wrote:

>>> Most of these test are for fixed point intrinsics. Maybe we should change the lowering of those to what we want instead of using a DAGCombine to get there?
>>
>> They are not affected by DAGCombine but changes in `Select`. Is this what you expected here?
>
> Why would it be the changes to Select? If the i8 or i16 umul_logo existed before this change, wouldn’t we have failed selection?

Not sure I understand your question here. I was thinking you mean to DAGCombine in X86ISelLowering, but now I guess you mean in DAGCombiner.
The code <https://github.com/llvm/llvm-project/commit/15090e1eb01ad> was there for a long time. We removed <https://github.com/llvm/llvm-project/commit/afa22edcf08ab18f8bd2d2b7412b65707b6da3e2> i8 and i16 several years ago due to that code.
So I think the answer to the question is we should give targets a chance to lower it themselves, hence calling `TLI.isOperationLegal(ISD::UMUL_LOHI, VT)` before doing that.
TBH, I didn't think that far when I changed the code, I just wanted to break the infinite combine loop between mul and mul_lohi.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5284
+  if (VT.isSimple() && !VT.isVector() &&
+      !TLI.isOperationLegal(ISD::UMUL_LOHI, VT)) {
     MVT Simple = VT.getSimpleVT();
----------------
craig.topper wrote:
> Shouldn't this be SMUL_LOHI?
Yes. Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153620