Reductions
Johannes Doerfert
jdoerfert at codeaurora.org
Fri Jun 13 11:14:29 PDT 2014
Fixed one line indicated by "make polly-check-format"
--
Johannes Doerfert
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation
-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu
[mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Johannes Doerfert
Sent: Friday, June 13, 2014 10:27 AM
To: 'Tobias Grosser'
Cc: 'Sebastian Pop'; llvm-commits at cs.uiuc.edu; polly-dev at googlegroups.com
Subject: RE: Reductions
Hey Tobias,
I changed almost everything according to your comments (except the prefix in
the test cases, I don't see the need to do this, but if you insist I will).
I added another "only reduction like" test case and a floating point test
case w and w/o fast math [= 4 test cases for in the first patch].
What is left in order to commit this one and get to the next patch?
Comments inlined (formatting might be a bit off, sorry about that).
--
Johannes Doerfert
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation
>> any reason you did not post these patches on llvm-commits@?
Old habits, I added llvm-commits now too. Afterwards we only reply to that
list.
>> > The patches add:
>> >
>> > 1) Very minimal reduction detection to the scop statements (one
>> > reduction per stmt)
>> >
>>
>> Nice. I don't expect any problems here.
I just wait on your green light ;)
>> > 2) Reduction dependency modeling (for these "reduction only"
>> > statements)
>>
>> OK, this will be interesting and it is important to get it right. By
reducing it to the minimal patch size, we can hopefully avoid any
distraction that could slow us down.
>>
>> > 3) A check and pragma in the IslAst (to allow to see the effects)
>>
>> I like the idea that we already check their presence in the context
>> of
AST generation.
>>
>> > All benchmarks are quite small and contain/update 2-3 test cases
>> > which will hopefully allow quick feedback.
>>
>> Thanks a lot!
>>
>> Further comments inline.
>>
>> > From b672baa6a8a8b2163e99ca3013d9031ad5dfa262 Mon Sep 17 00:00:00
>> > 2001
>> > From: Johannes Doerfert<jdoerfert at codeaurora.org>
>> > Date: Thu, 12 Jun 2014 11:43:02 -0700
>> > Subject: [PATCH 1/3] Detect and mark reduction like statements
>> >
>> > + flag to indicate reduction like statements cmd option to
>> > + (dis)allow multiplicative reduction opcodes two simple test cases
>> > ---
>> > include/polly/ScopInfo.h | 7 ++
>> > lib/Analysis/ScopInfo.cpp | 81
+++++++++++++++++++++-
>> > .../simplest_binary_reduction_w_constant.ll | 41 +++++++++++
>> > test/Reductions/simplest_binary_reduction_w_iv.ll | 42 +++++++++++
>> > 4 files changed, 170 insertions(+), 1 deletion(-)
>> > create mode 100644
test/Reductions/simplest_binary_reduction_w_constant.ll
>> > create mode 100644
>> > test/Reductions/simplest_binary_reduction_w_iv.ll
>> >
>> > diff --git a/include/polly/ScopInfo.h b/include/polly/ScopInfo.h
>> > index
>> > e85ec13..f80cd42 100644
>> > --- a/include/polly/ScopInfo.h
>> > +++ b/include/polly/ScopInfo.h
>> > @@ -270,6 +270,8 @@ class ScopStmt {
>> > MemoryAccessVec MemAccs;
>> > std::map<const Instruction *, MemoryAccess *>
>> > InstructionToAccess;
>> >
>> > + /// @brief Flag to indicate reduction statements bool
>> > + IsReductionLike = false;
>>
>> Why do you call this ReductionLike and not just reduction?
>>
>> Also, can you explain what it means to be a reduction. Is it
>> associative
_and_ commutative? Or just one of them? I would also mention the restriction
on two memory accesses. We later need to extend this to the multiple memory
access case.
I added a descriptive comment to this Boolean flag. It talks about reduction
like, associative, commutative, and the restriction.
Lifting the restriction soon is one of mine priorities too, but I'd like
positive feedback on these patches first.
>> > #include "isl/constraint.h"
>> > @@ -55,6 +55,12 @@ using namespace polly;
>> > STATISTIC(ScopFound, "Number of valid Scops");
>> > STATISTIC(RichScopFound, "Number of Scops containing a loop");
>> >
>> > +static cl::opt<bool>
>> >
+DisableMultiplicativeReductions("polly-disable-multiplicative-reduction
+s",
>> > + cl::desc("Disable multiplicative
reductions"),
>> > + cl::Hidden, cl::ZeroOrMore,
cl::init(false),
>> > + cl::cat(PollyCategory));
>>
>> Out of interest. Why would we want to disable those, but not the
>> other
possible operands?
They overflow easily, add is pretty safe and bit operations are stable too.
Is that OK. ?
>> > /// Translate a 'const SCEV *' expression in an isl_pw_aff.
>> > struct SCEVAffinator : public SCEVVisitor<SCEVAffinator,
>> > isl_pw_aff
*> {
>> > public:
>> > @@ -264,6 +270,48 @@ int SCEVAffinator::getLoopDepth(const Loop *L) {
>> > return L->getLoopDepth() - outerLoop->getLoopDepth();
>> > }
>> >
>> >
>> > //===--------------------------------------------------------------
>> > ---
>> > -----===//
>> >
>> > MemoryAccess::~MemoryAccess() {
>> > @@ -696,6 +744,36 @@ ScopStmt::ScopStmt(Scop &parent, TempScop
&tempScop, const Region &CurRegion,
>> > Domain = buildDomain(tempScop, CurRegion);
>> > buildScattering(Scatter);
>> > buildAccesses(tempScop, CurRegion);
>> > + checkForReduction();
>> > +}
>> > +
>> > +void ScopStmt::checkForReduction() {
>> > + // Skip statements with more than one binary reduction
>> > + if (MemAccs.size() != 2)
>> > + return;
>> > +
>> > + // Skip if there is not exactly one load and one store unsigned
>> > + LoadIdx = MemAccs[0]->isRead() ? 0 : 1; auto *Load =
>> > + dyn_cast<LoadInst>(MemAccs[LoadIdx]->getAccessInstruction());
>> > + auto *Store = dyn_cast<StoreInst>(MemAccs[1 -
>> > + LoadIdx]->getAccessInstruction());
>> > + if (!Load || !Store)
>> > + return;
>> > +
>> > + // Skip if the load has multiple uses if (Load->getNumUses() != 1)
>> > + return;
>> > +
>> > + // Skip if there is not one binary operator between the load and
>> > + the store auto *BinOp =
>> > + dyn_cast<BinaryOperator>(Store->getValueOperand());
>> > + if (!BinOp || (BinOp->getOperand(0) != Load &&
>> > + BinOp->getOperand(1)
!= Load))
>> > + return;
>> Should we also check that the load and store instruction point to the
same memory location (reference the very same pointer)?
Not in the current setting. I wanted it to be a small first patch so I use
reduction like and allow for example:
for I
for j
B[i] = A[i] + 3;
However there won't be any reduction dependences for this example so there
is no real harm done. Once we allow multiple accesses we have to check (at
least) for clobbering accesses and possibly also for the same base value
(while we can do this in the dependency analysis too).
For now I would suggest to keep the detection lightweight, we could however
discuss if we want to do have the checks needed in the future in the
dependency analysis or already during detection. In my opinion the detection
might be complicated enough once we remove all limits.
>> > + // Skip if the opcode of the binary operator is not valid if
>> > + (!hasValidOpCode(BinOp, Load))
>> > + return;
>>
>> Can we use the isAssociative/isCommutative() functions for now.
I changed it, but some functionality is now missing (sub/div on the right
side) and we still have to look into the opcode if we want to be able to
disable the multiplicative reductions.
>> > std::string ScopStmt::getDomainStr() const { return
>> > stringFromIslObj(Domain); } @@ -751,6 +829,7 @@
>> > ScopStmt::~ScopStmt() {
>> >
>> > void ScopStmt::print(raw_ostream &OS) const {
>> > OS << "\t" << getBaseName() << "\n";
>> > + OS.indent(8) << "Reduction like: " << isReductionLike() << "\n";
>> >
>> > OS.indent(12) << "Domain :=\n";
>> >
>> > diff --git
>> > a/test/Reductions/simplest_binary_reduction_w_constant.ll
>> > b/test/Reductions/simplest_binary_reduction_w_constant.ll
>> > new file mode 100644
>> > index 0000000..7ab9d47
>> > --- /dev/null
>> > +++ b/test/Reductions/simplest_binary_reduction_w_constant.ll
>>
>> This should be in test/ScopInfo. You can prefix these tests with
reduction_
Done.
>> > @@ -0,0 +1,41 @@
>> > +; RUN: opt %loadPolly -polly-scops -analyze -basicaa < %s |
>> > +FileCheck %s --check-prefix=DETECTION
>> Do you need -basicaa here?
Removed.
>> > +;
>> > +; DETECTION: Reduction like: 1
>> > +;
>> > +; int f(int sum) {
>> > +; int i;
>> > +; for (int i = 0; i <= 100; i++) {
>> > +; sum += 3;
>> > +; }
>> > +; return sum;
>> > +; }
>> > +;
>> > +; ModuleID = 'test/Reductions/simplest_binary_reduction.ll'
>> > +target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-n32-S64"
>> > +
>> > +; Function Attrs: nounwind
>> > +define i32 @f(i32 %sum) {
>> > +entry:
>> > + %sum.reg2mem = alloca i32
>> > + br label %entry.split1
>> > +
>> > +entry.split1: ; preds = %entry
>> > + br label %entry.split
>> > +
>> > +entry.split: ; preds =
%entry.split1
>> > + store i32 %sum, i32* %sum.reg2mem br label %for.cond
>> > +
>> > +for.cond: ; preds = %for.cond,
%entry.split
>> > + %i1.0 = phi i32 [ 0, %entry.split ], [ %inc, %for.cond ]
>> > + %sum.reload = load i32* %sum.reg2mem %add = add nsw i32
>> > + %sum.reload, 3 %inc = add nsw i32 %i1.0, 1 store i32 %add, i32*
>> > + %sum.reg2mem %cmp = icmp slt i32 %i1.0, 100 br i1 %cmp, label
>> > + %for.cond, label %for.end
>> > +
>> > +for.end: ; preds = %for.cond
>> > + %sum.reload.2 = load i32* %sum.reg2mem
>> > + ret i32 %sum.reload.2
>> > +}
>> > diff --git a/test/Reductions/simplest_binary_reduction_w_iv.ll
>> > b/test/Reductions/simplest_binary_reduction_w_iv.ll
>> > new file mode 100644
>> > index 0000000..6c49988
>> > --- /dev/null
>> > +++ b/test/Reductions/simplest_binary_reduction_w_iv.ll
>> > @@ -0,0 +1,42 @@
>> > +; RUN: opt %loadPolly -polly-scops -analyze -basicaa < %s |
>> > +FileCheck %s --check-prefix=DETECTION
>>
>> Do you need -basicaa here?
Removed.
>> > +;
>> > +; DETECTION: Reduction like: 1
>>
>> We commonly don't introduce a check prefix if there is just one run line.
I'll introduce more in future commits (already in the second one in this
change set).
Do I really need to change that now?
>> > +;
>> > +; int f(int sum) {
>> > +; int i;
>> > +; for (int i = 0; i <= 100; i++) {
>> > +; sum += i * 3;
>> > +; }
>> > +; return sum;
>> > +; }
>> > +;
>> > +; ModuleID = 'test/Reductions/simplest_binary_reduction.ll'
>> > +target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-n32-S64"
>> > +
>> > +; Function Attrs: nounwind
>> > +define i32 @f(i32 %sum) {
>> > +entry:
>> > + %sum.reg2mem = alloca i32
>> > + br label %entry.split1
>> > +
>> > +entry.split1: ; preds = %entry
>> > + br label %entry.split
>> > +
>> > +entry.split: ; preds =
%entry.split1
>> > + store i32 %sum, i32* %sum.reg2
--
You received this message because you are subscribed to the Google Groups
"Polly Development" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to polly-dev+unsubscribe at googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Detect-and-mark-reduction-like-statements-v3.patch
Type: application/octet-stream
Size: 12668 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140613/422bf335/attachment.obj>
More information about the llvm-commits
mailing list