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

[Ariel Ben-Yehuda via llvm-commits]

I don't see an easy way to keep the OrderedInstructions info valid
through all the passes that use computeKnownBits.

I'm not sure how much of a problem this is at these passes - we can
already go through quite a lot of stuff with the old
`isSafeToSpeculativelyExecute` (e.g., loads of `dereferencable` data +
arbitrary arithmetic). Because of that, I'm not inclined to believe
this will make that problem much worse in practice.

In any case I doubt any of this will show at clang, because clang does
not emit either a lot of calls to either assume or !range metadata.


On Sun, Sep 3, 2017 at 7:19 PM, Ariel Ben-Yehuda <ariel.byd at gmail.com> wrote:
> I'll try to see what I can do. Do you have any other pointers for me?
>
> On Sun, Sep 3, 2017 at 7:17 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>
>> 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
>>