[PATCH] D96413: [DAG] Move basic USUBSAT pattern matches from X86 to DAGCombine

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3149
+    // clamping RHS.
+    KnownBits KnownLHS = DAG.computeKnownBits(LHS);
+    unsigned NumZeros = KnownLHS.countMinLeadingZeros();
----------------
Is this equivalent to something like  

if (!DAG.MaskedValueIsZero(LHS, APInt::getBitsSetFrom(SubVT.getScalarSizeInBits(), DstVT.getScalarSizeInBits())


================
Comment at: llvm/test/CodeGen/AArch64/usub_sat.ll:35
+; CHECK-NEXT:    subs w8, w8, w1, uxth
+; CHECK-NEXT:    csel w0, wzr, w8, lo
 ; CHECK-NEXT:    ret
----------------
RKSimon wrote:
> craig.topper wrote:
> > RKSimon wrote:
> > > craig.topper wrote:
> > > > RKSimon wrote:
> > > > > craig.topper wrote:
> > > > > > nikic wrote:
> > > > > > > Why are tests that directly use usub.sat intrinsics affected by this? Are we doing something weird like first expanding them to min/max sub and then combining them back to usubsat on the extended type?
> > > > > > I think type legalization expands it when it promotes the type. For USUBSAT, the expansion isn't necessary. Promoting the operands by zero extending would have been enough since the saturating value is 0 so is not affected by the promoted type. This is different than UADDSAT where the saturation value is UINT_MAX of the original type.
> > > > > Its a (nice?) sideeffect of the args actually being passed as zeroext i32 - we start off with a usubsat i16, which gets promoted to a i32 expanded sequence, but we're still before legalops, and as we know we have zero'd upper bits the new combine reforms the usubsat i32 pattern, which then expands to the shorter i32 codegen.
> > > > I thought the zero upper bits is only checked if we look through a truncate.
> > > > 
> > > > If we change the type legalizer to preserve USUBSAT we pick up an improvement on at least one additional X86 test.
> > > Sorry my mistake - I was getting mixed and was using other targets tests against aarch64.
> > > 
> > > Are you proposing we emit the promoted USUBSAT inside DAGTypeLegalizer::PromoteIntRes_ADDSUBSHLSAT ?
> > Yes. It already zero extended the operands so it should be safe I think. It picks up a couple improvements to 	
> > X86/usub_sat_plus.ll that get broken by SimplifyDemandedBits before the combine in this patch has a chance to kick in.
> Yes I can do that - do you have any objection to me doing that as a follow-up patch for review after this one has landed?
No objection


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96413