Page MenuHomePhabricator

[PowerPC] Don't generate ST_VSR_SCAL_INT if power8-vector is disabled, fix PR45297
ClosedPublic

Authored by lkail on Wed, Mar 25, 7:00 AM.

Details

Summary

In https://bugs.llvm.org/show_bug.cgi?id=45297, it fails selecting instructions for PPCISD::ST_VSR_SCAL_INT. The reason it generate the PPCISD::ST_VSR_SCAL_INT with -power8-vector in IR is PPC's combiner checks hasP8Altivec rather than hasP8Vector. This patch should resolve PR45297.

Diff Detail

Event Timeline

lkail created this revision.Wed, Mar 25, 7:00 AM
lkail edited the summary of this revision. (Show Details)Wed, Mar 25, 7:08 AM

Can you fix the title of this? "Fix PR45297" is not very descriptive unless someone also reviews the PR.

lkail retitled this revision from [PowerPC] Fix PR45297 to [PowerPC] Don't generate ST_VSR_SCAL_INT if power8-vector is disabled, fix PR45297.Wed, Mar 25, 6:52 PM
lkail added a reviewer: jhibbits.
steven.zhang accepted this revision.Wed, Mar 25, 9:30 PM

LGTM. Please hold on for some days in case someone else has concern.

This revision is now accepted and ready to land.Wed, Mar 25, 9:30 PM

A couple minor suggestions. If you agree with them, feel free to land them as an NFC update to the test before commiting this patch.

llvm/test/CodeGen/PowerPC/pr45297.ll
1–6

Pre-commiting the test to show the intended change in behaviour is a really good idea. I think it would be more effective if we check for the error we are tying to fix though:

; RUN: not --crash llc -verify-machineinstrs \
; RUN:   -mtriple=powerpc64le-unknown-linux-gnu -mattr=+altivec \
; RUN:   -mattr=-power8-vector -mattr=-vsx < %s 2>&1 | FileCheck %s

; CHECK: LLVM ERROR: Cannot select: t{{[0-9]+}}: ch = PPCISD::ST_VSR_SCAL_INT<(store 4 into @Global)>
11

Nit: can we store to something other then undef? Maybe changing the IR to:

@Global = dso_local global i32 55, align 4

define dso_local void @test(float %0) local_unnamed_addr {
entry:
  %1 = fptosi float %0 to i32
  store i32 %1, i32* @Global, align 4
  ret void
}
lkail updated this revision to Diff 253043.Thu, Mar 26, 10:00 PM

Address @sfertile 's comment.

lkail marked 2 inline comments as done.Thu, Mar 26, 10:01 PM
lkail updated this revision to Diff 253044.Thu, Mar 26, 10:41 PM
sfertile accepted this revision.Fri, Mar 27, 8:16 AM

Thank you for the updates. One minor comment, but patch LGTM.

llvm/test/CodeGen/PowerPC/pr45297.ll
2

Do we want to use -ppc-asm-full-reg-names to make the assembly easier to read?

This revision was automatically updated to reflect the committed changes.