[PATCH] D21409: Emit a swap for STXVD2X when it's emitted by matching a 'store' node

[Tim Shen via llvm-commits]

timshen added a comment.

> Having said that, I have not done a full investigation of these patches. But from a quick look, with the other fix, for this bug, PPCTargetLowering::expandVSXStoreForLE() was invoked and this patch was not working. So the other patch seems to depend on the first path. The question is, with that patch do we still have cases, where this path of code is executed.


As the conclusion of my investigation, the other patch switches the lowering from the second path ((store v2f64:$XT, xoaddr:$dst) in the .td file above, which is also a fallback path when DAGCombiner::combine() fails) to the first path (PPCTargetLowering::expandVSXStoreForLE, generated by a successful combine()). This patch fixes the second path.

I guess the question is why do we have two paths? I haven't looked at FastISel yet.


================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:940
@@ -939,1 +939,3 @@
           (STXVD2X $rS, xoaddr:$dst)>;
+let Predicates = [IsLittleEndian] in {
+def : Pat<(store v2i64:$rS, xoaddr:$dst),
----------------
amehsan wrote:
> nemanjai wrote:
> > timshen wrote:
> > > nemanjai wrote:
> > > > timshen wrote:
> > > > > Without this change, powerpc le stxvd2x still works in most of the cases. Does that mean this line introduces a second code path to handle powerpc le stxvd2x? If so, can we remove the first one?
> > > > I am not sure which definitions you're referring to as first and second and what you're suggesting we remove. As you can see above, I've removed the DAG pattern that was part of the instruction definition.
> > > The DAG pattern "(store v2f64:$XT, xoaddr:$dst)" is for big endian, not little endian, because it doesn't consider swap at all.
> > > 
> > > By saying the first code path I mean PPCTargetLowering::expandVSXStoreForLE().
> > But both code paths now will emit a swap. The code in PPCTargetLowering::expandVSXStoreForLE() handles more than just this pattern you mentioned. I believe the code in this patch will only ever be exercised with fast isel, but I may be wrong about that though.
> This may or may not help to address the concern raised.
> 
> What we generate in PPCTargetLowering::expandVSXStoreForLE() is PPCISD::STXVD2X which is different from PPC::STXVD2X which is defined in the td file. 
> 
> I believe the code in this patch will only ever be exercised with fast isel, but I may be wrong about that though.

Does that mean, the STXVD2X selected by fast isel was entirely wrong before this patch? Wouldn't we have noticed that before?

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:940
@@ -939,1 +939,3 @@
           (STXVD2X $rS, xoaddr:$dst)>;
+let Predicates = [IsLittleEndian] in {
+def : Pat<(store v2i64:$rS, xoaddr:$dst),
----------------
timshen wrote:
> amehsan wrote:
> > nemanjai wrote:
> > > timshen wrote:
> > > > nemanjai wrote:
> > > > > timshen wrote:
> > > > > > Without this change, powerpc le stxvd2x still works in most of the cases. Does that mean this line introduces a second code path to handle powerpc le stxvd2x? If so, can we remove the first one?
> > > > > I am not sure which definitions you're referring to as first and second and what you're suggesting we remove. As you can see above, I've removed the DAG pattern that was part of the instruction definition.
> > > > The DAG pattern "(store v2f64:$XT, xoaddr:$dst)" is for big endian, not little endian, because it doesn't consider swap at all.
> > > > 
> > > > By saying the first code path I mean PPCTargetLowering::expandVSXStoreForLE().
> > > But both code paths now will emit a swap. The code in PPCTargetLowering::expandVSXStoreForLE() handles more than just this pattern you mentioned. I believe the code in this patch will only ever be exercised with fast isel, but I may be wrong about that though.
> > This may or may not help to address the concern raised.
> > 
> > What we generate in PPCTargetLowering::expandVSXStoreForLE() is PPCISD::STXVD2X which is different from PPC::STXVD2X which is defined in the td file. 
> > 
> > I believe the code in this patch will only ever be exercised with fast isel, but I may be wrong about that though.
> 
> Does that mean, the STXVD2X selected by fast isel was entirely wrong before this patch? Wouldn't we have noticed that before?
> What we generate in PPCTargetLowering::expandVSXStoreForLE() is PPCISD::STXVD2X which is different from PPC::STXVD2X which is defined in the td file.

This is helpful, thanks! Actually my patch D21416 fixes the test failure because it makes SelectionDAG successfully lower `store` to PPCISD::STXVD2X.


Repository:
  rL LLVM

http://reviews.llvm.org/D21409