[polly] r262033 - ScopDetect/Info: Add option to disable invariant load hoisting

[Tobias Grosser via llvm-commits]

On 02/27/2016 11:45 AM, Johannes Doerfert wrote:
> On 02/27, Tobias Grosser wrote:
>> On 02/27/2016 11:33 AM, Johannes Doerfert wrote:
>>> On 02/26, Tobias Grosser via llvm-commits wrote:
>>>> Author: grosser
>>>> Date: Fri Feb 26 10:43:35 2016
>>>> New Revision: 262033
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=262033&view=rev
>>>> Log:
>>>> ScopDetect/Info: Add option to disable invariant load hoisting
>>>>
>>>> This is helpful for test case reduction and other experiments.
>>>>
>>>> Modified:
>>>>      polly/trunk/include/polly/ScopDetection.h
>>>>      polly/trunk/lib/Analysis/ScopDetection.cpp
>>>>      polly/trunk/lib/Analysis/ScopInfo.cpp
>>>>
>>>> Modified: polly/trunk/include/polly/ScopDetection.h
>>>> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/include/polly/ScopDetection.h?rev=262033&r1=262032&r2=262033&view=diff
>>>> ==============================================================================
>>>> --- polly/trunk/include/polly/ScopDetection.h (original)
>>>> +++ polly/trunk/include/polly/ScopDetection.h Fri Feb 26 10:43:35 2016
>>>> @@ -110,6 +110,7 @@ extern bool PollyTrackFailures;
>>>>   extern bool PollyDelinearize;
>>>>   extern bool PollyUseRuntimeAliasChecks;
>>>>   extern bool PollyProcessUnprofitable;
>>>> +extern bool PollyInvariantLoadHoisting;
>>>>
>>>>   /// @brief A function attribute which will cause Polly to skip the function
>>>>   extern llvm::StringRef PollySkipFnAttr;
>>>>
>>>> Modified: polly/trunk/lib/Analysis/ScopDetection.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopDetection.cpp?rev=262033&r1=262032&r2=262033&view=diff
>>>> ==============================================================================
>>>> --- polly/trunk/lib/Analysis/ScopDetection.cpp (original)
>>>> +++ polly/trunk/lib/Analysis/ScopDetection.cpp Fri Feb 26 10:43:35 2016
>>>> @@ -171,6 +171,12 @@ static cl::opt<bool>
>>>>                   cl::Hidden, cl::init(false), cl::ZeroOrMore,
>>>>                   cl::cat(PollyCategory));
>>>>
>>>> +bool polly::PollyInvariantLoadHoisting;
>>>> +static cl::opt<bool, true> XPollyInvariantLoadHoisting(
>>>> +    "polly-invariant-load-hoisting", cl::desc("Hoist invariant loads."),
>>>> +    cl::location(PollyInvariantLoadHoisting), cl::Hidden, cl::ZeroOrMore,
>>>> +    cl::init(true), cl::cat(PollyCategory));
>>>> +
>>>>   /// @brief The minimal trip count under which loops are considered unprofitable.
>>>>   static const unsigned MIN_LOOP_TRIP_COUNT = 8;
>>>>
>>>> @@ -304,6 +310,9 @@ bool ScopDetection::onlyValidRequiredInv
>>>>       InvariantLoadsSetTy &RequiredILS, DetectionContext &Context) const {
>>>>     Region &CurRegion = Context.CurRegion;
>>>>
>>>> +  if (!PollyInvariantLoadHoisting && !RequiredILS.empty())
>>>> +    return false;
>>>> +
>>>>     for (LoadInst *Load : RequiredILS)
>>>>       if (!isHoistableLoad(Load, CurRegion, *LI, *SE))
>>>>         return false;
>>>>
>>>> Modified: polly/trunk/lib/Analysis/ScopInfo.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopInfo.cpp?rev=262033&r1=262032&r2=262033&view=diff
>>>> ==============================================================================
>>>> --- polly/trunk/lib/Analysis/ScopInfo.cpp (original)
>>>> +++ polly/trunk/lib/Analysis/ScopInfo.cpp Fri Feb 26 10:43:35 2016
>>>> @@ -3116,27 +3116,28 @@ void Scop::verifyInvariantLoads(ScopDete
>>>>   }
>>>>
>>>>   void Scop::hoistInvariantLoads(ScopDetection &SD) {
>>>> -  isl_union_map *Writes = getWrites();
>>>> -  for (ScopStmt &Stmt : *this) {
>>>> -
>>>> -    MemoryAccessList InvariantAccesses;
>>>> -
>>>> -    for (MemoryAccess *Access : Stmt)
>>>> -      if (isHoistableAccess(Access, Writes))
>>>> -        InvariantAccesses.push_front(Access);
>>>> -
>>>> -    // We inserted invariant accesses always in the front but need them to be
>>>> -    // sorted in a "natural order". The statements are already sorted in reverse
>>>> -    // post order and that suffices for the accesses too. The reason we require
>>>> -    // an order in the first place is the dependences between invariant loads
>>>> -    // that can be caused by indirect loads.
>>>> -    InvariantAccesses.reverse();
>>>> -
>>>> -    // Transfer the memory access from the statement to the SCoP.
>>>> -    Stmt.removeMemoryAccesses(InvariantAccesses);
>>>> -    addInvariantLoads(Stmt, InvariantAccesses);
>>>> +  if (PollyInvariantLoadHoisting) {
>>>> +    isl_union_map *Writes = getWrites();
>>>> +    for (ScopStmt &Stmt : *this) {
>>>> +      MemoryAccessList InvariantAccesses;
>>>> +
>>>> +      for (MemoryAccess *Access : Stmt)
>>>> +        if (isHoistableAccess(Access, Writes))
>>>> +          InvariantAccesses.push_front(Access);
>>>> +
>>>> +      // We inserted invariant accesses always in the front but need them to be
>>>> +      // sorted in a "natural order". The statements are already sorted in
>>>> +      // reverse post order and that suffices for the accesses too. The reason
>>>> +      // we require an order in the first place is the dependences between
>>>> +      // invariant loads that can be caused by indirect loads.
>>>> +      InvariantAccesses.reverse();
>>>> +
>>>> +      // Transfer the memory access from the statement to the SCoP.
>>>> +      Stmt.removeMemoryAccesses(InvariantAccesses);
>>>> +      addInvariantLoads(Stmt, InvariantAccesses);
>>>> +    }
>>>> +    isl_union_map_free(Writes);
>>>>     }
>>>> -  isl_union_map_free(Writes);
>>>>
>>>>     verifyInvariantLoads(SD);
>>>>   }
>>>
>>> Just for the sake of completeness:
>>>
>>> if (!PollyInvariantLoadHoisting) {
>>>    verifyInvariantLoads(SD);
>>>    return;
>>> }
>>>
>>> Would have allowed to keep the old (smaller) indention.
>>
>> I am confused. How is this identical?
>>
>> I don't want to skip verification, I just want to avoid hoisting loads. It
>> seems your patch does still hoist loads, but then does not verify if all
>> loads have been hoisted.
> No it does not. Revert your patch and add these 4 lines at the beginning
> of the method. It will verify and leave the method if
> PollyInvariantLoadHoisting is false and otherwise behave as before
> (since we did not change anything later).

Good point. I also removed verifyInvariantLoads out of the function to 
void code duplication: r262203

Thanks for your comment.

Best,
Tobias