This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC, DAGCombiner] Fold a << (b % (sizeof(a) * 8)) back to a single instruction
ClosedPublic

Authored by timshen on Jan 4 2017, 2:57 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

timshen updated this revision to Diff 83149.Jan 4 2017, 2:57 PM
timshen retitled this revision from to [PowerPC, DAGCombiner] Fold a << (b % (sizeof(a) * 8)) back to a single instruction.
timshen updated this object.
timshen added reviewers: bogner, kbarton, hfinkel, iteratee.
timshen added a subscriber: llvm-commits.
nemanjai added inline comments.Jan 4 2017, 11:13 PM
llvm/lib/Target/PowerPC/PPCISelLowering.h
999 ↗(On Diff #83149)

I don't think this is the case with every shift instruction. Please see the test case below.

llvm/test/CodeGen/PowerPC/shift_mask.ll
25 ↗(On Diff #83149)

The results before and after when 31 < b < 64 are different. I don't think this is a safe transformation in this case. The reason being that as the ISA states:

Shift amounts from 32 to 63 give a zero result.

timshen updated this revision to Diff 83197.Jan 5 2017, 12:00 AM

Don't mis-optimize non-vector cases.

timshen updated this revision to Diff 83203.Jan 5 2017, 12:58 AM
timshen marked 2 inline comments as done.

Only optimize legal instructions.

I feel like needing a separate patch to add some test cases for illegal vector instructions (that get lowered to scalar instructions), and those shouldn't be combined. I need to investigate how to make those instructions illegal.

nemanjai added inline comments.Jan 5 2017, 2:03 AM
llvm/lib/Target/PowerPC/PPCISelLowering.h
1004 ↗(On Diff #83203)

I think this will exclude the scalar versions as well as any that will be scalarized. However, it'll also exclude versions that will be legalized in different ways (i.e. without scalarization). Wouldn't we want something that more closely reflects what we are looking for (i.e. the type will remain a vector after legalization)? Perhaps something along the lines of:

Index: lib/Target/PowerPC/PPCISelLowering.h
===================================================================
--- lib/Target/PowerPC/PPCISelLowering.h        (revision 290968)
+++ lib/Target/PowerPC/PPCISelLowering.h        (working copy)
@@ -996,6 +996,14 @@ namespace llvm {
     SDValue
       combineElementTruncationToVectorTruncation(SDNode *N,
                                                  DAGCombinerInfo &DCI) const;
+
+    bool supportsModuloShift(ISD::NodeType Inst, EVT ReturnType,
+                             const SelectionDAG &DAG) const override {
+      assert((Inst == ISD::SHL || Inst == ISD::SRA || Inst == ISD::SRL) &&
+             "Expect a shift instruction");
+      EVT LegalizedType = getTypeToTransformTo(*DAG.getContext(), ReturnType);
+      return isOperationLegal(Inst, LegalizedType) && LegalizedType.isVector();
+    }
   };
 
   namespace PPC {
timshen added inline comments.Jan 5 2017, 2:07 PM
llvm/lib/Target/PowerPC/PPCISelLowering.h
1004 ↗(On Diff #83203)

+ EVT LegalizedType = getTypeToTransformTo(*DAG.getContext(), ReturnType);
+ return isOperationLegal(Inst, LegalizedType) && LegalizedType.isVector();

Let's say isOperationLegal(Inst, LegalizedType) && LegalizedType.isVector(); is true, and we do the combine. However, later Inst turns into Inst' during the legalization, where Inst' might be any instruction that carries no modulo semantic. Won't that be a mis-combine?

nemanjai added inline comments.Jan 10 2017, 2:10 AM
llvm/lib/Target/PowerPC/PPCISelLowering.h
1004 ↗(On Diff #83203)

I'm not sure I follow why the node would be transformed to something else if type legalization will legalize the type to LegalizedType and we've already determined that the operation is legal.

In any case, it seems like we might want to run this combine only after legalization. I think that in that case, we'd get the semantics we're after. And I think it makes sense for other targets too as the target may not be able to definitively say whether it supports modulo shift semantics until it knows everything about the operation and type.

timshen updated this revision to Diff 84227.Jan 12 2017, 10:51 PM
timshen marked an inline comment as done.

Move the legalization check to the caller sides.

llvm/lib/Target/PowerPC/PPCISelLowering.h
1004 ↗(On Diff #83203)

Agreed. I move the legalization check to the caller sides.

iteratee accepted this revision.Jan 31 2017, 12:57 PM

It looks fine to me.

This revision is now accepted and ready to land.Jan 31 2017, 12:57 PM

Did you ever commit this?

This revision was automatically updated to reflect the committed changes.