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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
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
----------------
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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96413