[PATCH] D37216: [SROA] propagate !range metadata when moving loads

[Hal Finkel via llvm-commits]

On 09/03/2017 11:12 AM, Ariel Ben-Yehuda wrote:
> I think the performance worry is already a problem with my previous
> patch - we already call isValidAssumeForContext quite a bit of times
> in ValueTracking, where it will perform the long walk on a big BB.

Indeed. Maybe we can give 
include/llvm/Transforms/Utils/OrderedInstructions.h a mode in which it 
keeps track of guaranteed-execution regions? Then we can check for 
region membership and dominance in one step.

Thanks again,
Hal

>
> On Sun, Sep 3, 2017 at 6:58 PM, Hal Finkel via Phabricator
> <reviews at reviews.llvm.org> wrote:
>> hfinkel added a comment.
>>
>> This looks good, but, have you checked for compile-time regressions? I'm concerned here because:
>>
>> We've now made addAssumptionsFromMetadata linear in the number of instructions between the instruction and its replacement. That should be documented with the function declaration (at least). As the replacement always dominates the original instruction, when they're in the same BB, we'll always be doing that linear walk. As you're making the walk go through normal loads, I can imagine this becoming quadratic in some inputs (unless there's something about the way that SROA uses this that prevents that, and if so, we need to document that too).
>>
>> If this can become quadratic, I think we'll want to maintain some kind of BB-region analysis, there we partition each BB into regions based on guaranteed execution, and then use that instead of walking.
>>
>>
>>
>> ================
>> Comment at: include/llvm/Analysis/ValueTracking.h:375
>> +  /// Return true if, when CxtI executes, it is guaranteed that either
>> +  /// I had executed already or that I is guaranteed to be reached and
>> +  /// be executed.
>> ----------------
>> reached and executed -> later executed
>>
>>
>> ================
>> Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:314
>> +                                      AssumptionCache *AC)
>> +{
>> +  if (LI->getMetadata(LLVMContext::MD_nonnull) &&
>> ----------------
>> Brace on previous line.
>>
>>
>> ================
>> Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:316
>> +  if (LI->getMetadata(LLVMContext::MD_nonnull) &&
>> +      !llvm::isKnownNonNullAt(ReplVal, LI, &DT)) {
>> +    addAssumeNonNull(AC, LI);
>> ----------------
>> Do you need the llvm:: here? If not, please remove it.
>>
>>
>> https://reviews.llvm.org/D37216
>>
>>
>>

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory