Page MenuHomePhabricator

[PATCH] FastISel fixes with FeatureVSX enabled
ClosedPublic

Authored by seurer on Sep 15 2014, 2:20 PM.

Details

Summary

Test case and FastISel fixes with FeatureVSX enabled

There are a number of test cases that fail if FeatureVSX is turned on.

Many of them use options that aren't compatible with VSX (like -m32 or darwin
targets). I just disabled vsx (via -mattr=-vsx) for these.

The rest are looking for specific instructions that are not generated with
FeatureVSX enabled. I added -mattr=-vsx to the existing test checks for these
and then added new checks specific to VSX to look for the VSX instructions that
are generated instead.

The FastISel code cannot handle VSX registers and various assertion checks
will fail if one is encountered when it runs. I changed all the places in
FastISEL that allocate registers to check for VSX registers and return false
if one is found so that the regular code gen can handle it.

Diff Detail

Event Timeline

seurer updated this revision to Diff 13731.Sep 15 2014, 2:20 PM
seurer retitled this revision from to [PATCH] Test case and FastISel fixes with FeatureVSX enabled.
seurer updated this object.
seurer edited the test plan for this revision. (Show Details)
seurer added a subscriber: Unknown Object (MLST).Sep 15 2014, 2:23 PM
mcrosier added a reviewer: rkotler.
mcrosier removed a subscriber: mcrosier.

Added Daniel and Reed as reviews.

echristo added a subscriber: echristo.

Wrong reviewers. :)

This is PPC you want one of these.

echristo edited edge metadata.Sep 15 2014, 3:27 PM

This should probably all be handled via the isTypeLegal machinery rather than this cut and paste.

Thanks!

-eric

hfinkel edited edge metadata.Sep 19 2014, 5:17 AM

Thanks for working on this!

Regarding approach, instead of disabling parts of FastISel when VSX is enabled, why don't we disable VSX when FastISel would be used? For example, what if in PPCSubtarget::initSubtargetFeatures we added something like this after the call to ParseSubtargetFeatures:

if (OptLevel == CodeGenOpt::None) or even better if possible (TM.Options.EnableFastISel)
  HasVSX = false;

(we already do something similar for little-Endian mode currently). Then we just need to add an assert in the FastISel implementation that VSX is not enabled.

As a general note, please upload full-context diffs in the future (see http://llvm.org/docs/Phabricator.html for instructions).

In D5362#12, @hfinkel wrote:

Thanks for working on this!

Regarding approach, instead of disabling parts of FastISel when VSX is enabled, why don't we disable VSX when FastISel would be used? For example, what if in PPCSubtarget::initSubtargetFeatures we added something like this after the call to ParseSubtargetFeatures:

if (OptLevel == CodeGenOpt::None) or even better if possible (TM.Options.EnableFastISel)
  HasVSX = false;

(we already do something similar for little-Endian mode currently). Then we just need to add an assert in the FastISel implementation that VSX is not enabled.

How about changing getRegForValue to return 0 if it ends up with a VSX register. All the uses of getRegForValue already check for a 0 return value. That moves the code to just one spot and there are already multiple checks there for things that FastISel can't handle.

As a general note, please upload full-context diffs in the future (see http://llvm.org/docs/Phabricator.html for instructions).

Will do.

seurer updated this revision to Diff 14458.Oct 6 2014, 9:11 AM
seurer updated this object.
seurer edited edge metadata.

New IsTypeLegal function added to FastISel
Overrides for IsTypeLegal added to PPCFastIsel
Some additional test case updates, mostly with option changes

wschmidt edited edge metadata.Oct 6 2014, 10:44 AM

Hi Bill,

This looks much better with the checking all in one place -- thanks for doing that! I don't see any problems with the patch from my end -- will defer to Hal and Eric whether their concerns are all handled.

I see we will have an opportunity to improve code gen for mcm-12.ll. Using the indexed-form VSX instruction allows use of more registers but means we need an extra addi in this sequence over just using lfd. But that's for the future...

hfinkel added inline comments.Oct 6 2014, 2:00 PM
/home/seurer/llvm/llvm-test/include/llvm/CodeGen/FastISel.h
486 ↗(On Diff #14458)

Do other targets break if you make this call their two-argument isTypeLegal with a throw-away VT argument?

I'd rather do this in a way that targets don't need to provide two very-similar overrides and keep them in sync if possible.

/home/seurer/llvm/llvm-test/lib/Target/PowerPC/PPCFastISel.cpp
264 ↗(On Diff #14458)

Not sure about f128 (it is not a legal type, and I assume the base class will take care of this), but if ppcf128 should be here, please add it.

hfinkel accepted this revision.Oct 6 2014, 4:36 PM
hfinkel edited edge metadata.
This revision is now accepted and ready to land.Oct 6 2014, 4:36 PM
seurer updated this revision to Diff 14514.Oct 7 2014, 8:38 AM
seurer edited edge metadata.

Added const to isTypeLegal functions
Removed comment about f128 types

seurer updated this revision to Diff 14591.Oct 8 2014, 1:16 PM

Removed default FeatureVSX in PPC.td
Two changes in FastISel.cpp to avoid svn code merge problems

After discussing this with Bill Schmidt we decided to not turn on FeatureVSX by default. There are some recent code updates elsewhere (unrelated to this) which cause register issues when FeatureVSX is turned on.

Out of curiosity, why not this patch?

dzur:~/sources/llvm> git diff
diff --git a/lib/Target/PowerPC/PPCFastISel.cpp
b/lib/Target/PowerPC/PPCFastISel.cpp
index 99aa4ea..b36c5c8 100644

  • a/lib/Target/PowerPC/PPCFastISel.cpp

+++ b/lib/Target/PowerPC/PPCFastISel.cpp
@@ -2284,7 +2284,7 @@ namespace llvm {

// Only available on 64-bit ELF for now.
const PPCSubtarget *Subtarget = &TM.getSubtarget<PPCSubtarget>();
  • if (Subtarget->isPPC64() && Subtarget->isSVR4ABI())

+ if (Subtarget->isPPC64() && Subtarget->isSVR4ABI() && !Subtarget->hasVSX())

  return new PPCFastISel(FuncInfo, LibInfo);

return nullptr;
seurer updated this revision to Diff 14601.Oct 8 2014, 1:46 PM

Changes to fma.ll to fix merging

Not such a huge fan of the new EVT code. This is what the Type *, MVT
& code is for. What's giving you an EVT where you're not checking for
simple?

I.e. I see no reason this shouldn't work like:

bool PPCFastISel::isTypeLegal(Type *Ty, MVT &VT) {

EVT E = TLI.getValueType(Ty, true);

// Only handle simple types.
if (E == MVT::Other || !E.isSimple())
  return false;
VT = E.getSimpleVT();

if (VT == MVT::f64 || VT.isVector())
  return false;

return TLI.isTypeLegal(VT);

}

(though do we handle f64 or vectors if we don't have VSX? Do we need
both checks? etc)

-eric

OK, after some more looking I see where you're going with this. I
think the various bits can be unified under the isTypeLegal I brought
up before and making that virtual with the simple check it looks like
all of them check for MVT::isSimple() first.. You might need to pull
out the Other check though.

Few comments on the patch otherwise:

+++ /home/seurer/llvm/llvm-test/test/CodeGen/Generic/select-cc.ll
@@ -1,4 +1,4 @@
-; RUN: llc < %s
+; RUN: llc -mattr=-vsx < %s

This is a generic codegen test. You can't change it this way.

+++ /home/seurer/llvm/llvm-test/test/CodeGen/PowerPC/2007-09-08-unaligned.ll
@@ -1,7 +1,11 @@
-; RUN: llc < %s | grep stfd | count 3
-; RUN: llc < %s | grep stfs | count 1
-; RUN: llc < %s | grep lfd | count 2
-; RUN: llc < %s | grep lfs | count 2
+; RUN: llc -mattr=-vsx < %s | grep stfd | count 3
+; RUN: llc -mattr=-vsx < %s | grep stfs | count 1
+; RUN: llc -mattr=-vsx < %s | grep lfd | count 2
+; RUN: llc -mattr=-vsx < %s | grep lfs | count 2
+; RUN: llc -mattr=+vsx < %s | grep stxsdx | count 3
+; RUN: llc -mattr=+vsx < %s | grep stfs | count 1
+; RUN: llc -mattr=+vsx < %s | grep lxsdx | count 2
+; RUN: llc -mattr=+vsx < %s | grep lfs | count 2

This appears unrelated to fast-isel?

+++ /home/seurer/llvm/llvm-test/test/CodeGen/PowerPC/2012-10-12-bitcast.ll
@@ -1,4 +1,5 @@
-; RUN: llc -mattr=+altivec < %s | FileCheck %s
+; RUN: llc -mattr=-vsx -mattr=+altivec < %s | FileCheck %s
+; RUN: llc -mattr=+vsx -mattr=+altivec < %s | FileCheck -check-prefix=VSX %s

Ditto.

+++ /home/seurer/llvm/llvm-test/test/CodeGen/PowerPC/buildvec_canonicalize.ll
@@ -1,4 +1,5 @@
-; RUN: llc < %s -march=ppc32 -mattr=+altivec --enable-unsafe-fp-math

FileCheck %s

+; RUN: llc < %s -mattr=-vsx -march=ppc32 -mattr=+altivec
--enable-unsafe-fp-math | FileCheck %s
+; RUN: llc < %s -mattr=+vsx -march=ppc32 -mattr=+altivec
--enable-unsafe-fp-math | FileCheck -check-prefix=VSX %s

Ditto.

+++ /home/seurer/llvm/llvm-test/test/CodeGen/PowerPC/copysignl.ll
@@ -1,4 +1,5 @@
-; RUN: llc -mcpu=pwr7 -mtriple=powerpc64-unknown-linux-gnu < %s | FileCheck %s
+; RUN: llc -mcpu=pwr7 -mtriple=powerpc64-unknown-linux-gnu
-mattr=-vsx < %s | FileCheck %s -check-prefix=NOVSX
+; RUN: llc -mcpu=pwr7 -mtriple=powerpc64-unknown-linux-gnu
-mattr=+vsx < %s | FileCheck %s -check-prefix=VSX

Ditto.

+++ /home/seurer/llvm/llvm-test/test/CodeGen/PowerPC/fabs.ll
@@ -1,4 +1,5 @@
-; RUN: llc < %s -march=ppc32 -mtriple=powerpc-apple-darwin | grep "fabs f1, f1"
+; RUN: llc < %s -mattr=-vsx -march=ppc32
-mtriple=powerpc-apple-darwin | grep "fabs f1, f1"
+; RUN: llc < %s -mattr=+vsx -march=ppc32
-mtriple=powerpc-apple-darwin | grep "xsabsdp f1, f1"

Ditto.

+++ /home/seurer/llvm/llvm-test/test/CodeGen/PowerPC/fast-isel-call.ll
@@ -1,4 +1,4 @@
-; RUN: llc < %s -O0 -verify-machineinstrs -fast-isel-abort
-mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 | FileCheck %s
--check-prefix=ELF64
+; RUN: llc < %s -O0 -verify-machineinstrs -mattr=-vsx
-fast-isel-abort -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 |
FileCheck %s --check-prefix=ELF64

Do we really want to change almost every pwr7 fast isel test to remove
the vsx attribute just because bits of the tests don't work? I'm not
sure I've got a better solution, but this seems really heavy handed.

-; ELF64-LABEL: ret2
+; ELF64: ret2

Seems an orthogonal change?

+++ /home/seurer/llvm/llvm-test/test/CodeGen/PowerPC/fma.ll
@@ -1,4 +1,4 @@
-; RUN: llc < %s -march=ppc32 -fp-contract=fast | FileCheck %s
+; RUN: llc < %s -mattr=-vsx -march=ppc32 -fp-contract=fast | FileCheck %s

This appears unrelated to fast-isel? (Along with everything that
doesn't use -O0 on the llc command line).

+++ /home/seurer/llvm/llvm-test/test/CodeGen/PowerPC/fsel.ll
@@ -1,5 +1,6 @@
-; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 | FileCheck %s
-; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7
-enable-no-infs-fp-math -enable-no-nans-fp-math | FileCheck
-check-prefix=CHECK-FM %s
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7
-mattr=-vsx | FileCheck %s
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7
-enable-no-infs-fp-math -enable-no-nans-fp-math -mattr=-vsx |
FileCheck -check-prefix=CHECK-FM %s
+; RUN: llc < %s -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7
-enable-no-infs-fp-math -enable-no-nans-fp-math -mattr=+vsx |
FileCheck -check-prefix=CHECK-FM-VSX %s
target datalayout =
"E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f128:128:128-v128:128:128-n32:64"
target triple = "powerpc64-unknown-linux-gnu"

@@ -16,6 +17,10 @@
; CHECK-FM: @zerocmp1
; CHECK-FM: fsel 1, 1, 2, 3
; CHECK-FM: blr
+
+; CHECK-FM-VSX: @zerocmp1
+; CHECK-FM-VSX: fsel 1, 1, 2, 3
+; CHECK-FM-VSX: blr

Another orthogonal test change?

There's more seemingly of these as well, but calling out each one
separately seems to be a bit verbose.

Thoughts?

-eric

seurer updated this revision to Diff 14869.Oct 14 2014, 8:33 AM
seurer retitled this revision from [PATCH] Test case and FastISel fixes with FeatureVSX enabled to [PATCH] FastISel fixes with FeatureVSX enabled.

As per the discussion I have changed this to only contain the code fixes.

The current diff looks fine to me, and I'd be happy to commit it for you. Eric, I had difficulty parsing your last comment, but I believe this follows the direction you were pursuing. Any problem with moving forward on this one?

/me adds a light and airy *ping* to the discussion thread. ;)

I've been thinking about this a bit more, and I'm not very happy with a solution that changes the general FastISel machinery, given that it's intended to be temporary. I think seurer is making pretty good progress on the real solution for VSX in FastISel, so it might be best to suck it up and wait for that to be completed. Thoughts?

BTW, from a note he sent me this morning I have a feeling that FastISel may be ignoring AddedComplexity in the pattern selection code. The VSX pattern should be preferred over the other when available, but the non-VSX pattern code is being generated first, which doesn't give the VSX pattern a chance. Eric, is this possible?

In D5362#35, @wschmidt wrote:

I've been thinking about this a bit more, and I'm not very happy with a solution that changes the general FastISel machinery, given that it's intended to be temporary. I think seurer is making pretty good progress on the real solution for VSX in FastISel, so it might be best to suck it up and wait for that to be completed. Thoughts?

This is definitely my preference here. As a temporary solution I'm absolutely ok with checking if VSX is enabled and just disabling fastisel.

BTW, from a note he sent me this morning I have a feeling that FastISel may be ignoring AddedComplexity in the pattern selection code. The VSX pattern should be preferred over the other when available, but the non-VSX pattern code is being generated first, which doesn't give the VSX pattern a chance. Eric, is this possible?

I'm fairly certain we don't pay attention to AddedComplexity in fast isel. One solution would be to change the order of those patterns in the file I think. At least might be worth a try.

-eric

seurer closed this revision.Dec 8 2014, 6:57 AM

Superseded by [llvm] r223507