Page MenuHomePhabricator

[PowerPC] Legalize SREM/UREM directly on P9.
ClosedPublic

Authored by Esme on Jun 18 2020, 11:02 PM.

Details

Summary

As Bugzilla 35090 reported, the rationale for using custom lowering SREM/UREM should no longer be true. At the IR level, the div-rem-pairs pass rL312862 performs the transformation where the remainder is computed from the result of the division when both a required. We should now be able to lower these directly on P9. And the pass also fixed the problem that divide is in a different block than the remainder.
This is a NFC patch to remove redundant code.

Diff Detail

Event Timeline

Esme created this revision.Jun 18 2020, 11:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2020, 11:02 PM
lkail added a comment.Jun 23 2020, 7:14 PM

It looks not an NFC patch to me, the test predicates are changed

CHECK: modsw -> CHECK-NOT: modsw

I'm curious if it is unchanged that opt is added to RUN lines while the OperationAction keeps Custom. If so, adding opt to RUN lines can be an NFC patch, and making OperationAction Legal can be another FC patch.

Esme added a comment.Jun 23 2020, 7:41 PM

It looks not an NFC patch to me, the test predicates are changed

CHECK: modsw -> CHECK-NOT: modsw

I'm curious if it is unchanged that opt is added to RUN lines while the OperationAction keeps Custom. If so, adding opt to RUN lines can be an NFC patch, and making OperationAction Legal can be another FC patch.

Adding opt changed the predictions because the pass div-rem-pairs fixed the case @blocks_modulo_div_sw. If the pass is already added, making OperationAction Legal will not cause any change. I will separate the patch to 2 patches to make this clear.

lkail added a comment.Jun 23 2020, 8:16 PM

Better keep original RUN lines and adding new RUN with opt added and check prefix named like CHECK-DIV-REM-PAIRS.

Esme added a comment.Jun 23 2020, 11:15 PM

Better keep original RUN lines and adding new RUN with opt added and check prefix named like CHECK-DIV-REM-PAIRS.

I am not sure whether this is preferable. If so, the predictions will change after making OperationAction Legal...

I am not sure whether this is preferable. If so, the predictions will change after making OperationAction Legal...

Maybe I'm not making myself clear. We can make adding opt RUN lines in llvm/test/CodeGen/PowerPC/ppc64-P9-mod.ll an NFC patch

; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 -verify-machineinstrs | FileCheck %s
; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 -verify-machineinstrs | FileCheck %s
; RUN: llc < %s -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 | FileCheck %s -check-prefix=CHECK-PWR8 -implicit-check-not mod[us][wd]
; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 | \
; RUN:   llc -verify-machineinstrs | FileCheck -check-prefix=CHECK-DIV-REM-PAIRS %s
; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 | \
; RUN:   llc -verify-machineinstrs | FileCheck -check-prefix=CHECK-DIV-REM-PAIRS %s
; RUN: opt < %s -div-rem-pairs -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr8 | \
; RUN:   llc -verify-machineinstrs | FileCheck %s -check-prefix=CHECK-PWR8-DIV-REM-PAIRS -implicit-check-not mod[us][wd]

The second patch, we make actions Legal and compare the result of the NFC patch.

Esme updated this revision to Diff 273988.Jun 28 2020, 7:31 PM

Update predictions for test cases.

lkail accepted this revision.Jul 1 2020, 11:24 PM

LGTM.

This revision is now accepted and ready to land.Jul 1 2020, 11:24 PM
lkail added a comment.Jul 1 2020, 11:47 PM

Please remove 'NFC' from the title when landing this patch.

Esme retitled this revision from [NFC][PowerPC] Legalize SREM/UREM directly on P9. to [PowerPC] Legalize SREM/UREM directly on P9..Mon, Jul 6, 3:37 AM
This revision was automatically updated to reflect the committed changes.