[PATCH] D122968: [AArch64][SelectionDAG] Add target-specific implementation of srem

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM with one minor comment



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:422
     SDValue visitSDIVLike(SDValue N0, SDValue N1, SDNode *N);
+    SDValue visitSREM(SDValue N0, SDValue N1, SDNode *N);
     SDValue visitUDIV(SDNode *N);
----------------
Please rename this so it's clear it's not the primary visitor function for SREM.  Maybe call it "buildOptimizedSREM" or something like that.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4582
+        return OptimizedRem;
+    }
     SDValue OptimizedDiv =
----------------
bcl5980 wrote:
> efriedma wrote:
> > bcl5980 wrote:
> > > bcl5980 wrote:
> > > > bcl5980 wrote:
> > > > > efriedma wrote:
> > > > > > Do we need to do something special for something like the following, to match the existing handling for div/rem pairs?  I guess it's only a couple instructions different either way, but maybe worth considering.
> > > > > > 
> > > > > > ```
> > > > > > define void @sdivrem(i32 %x, i32* %ap, i32* %bp) {
> > > > > >   %a = sdiv i32 %x, 4
> > > > > >   %b = srem i32 %x, 4
> > > > > >   store i32 %a, i32* %ap
> > > > > >   store i32 %b, i32* %bp
> > > > > >   ret void
> > > > > > }
> > > > > > ```
> > > > > For now I have no AArch64 device to test, do we have some tools to verify instructions similar to alive2.llvm.org/ce/ on AArch64?
> > > > > If no maybe I should use qemu to build a test environment.
> > > > I use IR to write the logic. Is this the behavior you want?
> > > >  https://alive2.llvm.org/ce/z/hGZBBk
> > > Based on the IR to asm by llc, it looks sdiv/srem pair can't get benifit from this pattern.
> > > https://godbolt.org/z/Pns5YvYfY
> > > 
> > > And it looks we lack dag combine from neg + cmp to negs based on the test.
> > I just meant that I wasn't sure if we would end up with the same code sequence as we would without this patch; I wasn't suggesting any specific transform.  (I guess what happens might be sensitive to whether we visit the SDIV or the SREM first...)
> I check the !DAG.doesNodeExist(ISD::SDIV, N->getVTList(), {N0, N1}) at line 4412,  SDIV / SREM paire won't go into the srem only path
Oh, I see; that's fine, then.


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

https://reviews.llvm.org/D122968