[PATCH] D37215: [ValueTracking] improve reverse assumption inference

[Ariel Ben-Yehuda via llvm-commits]

Done

On Fri, Sep 1, 2017 at 12:05 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> On 08/31/2017 03:58 PM, Ariel Ben-Yehuda wrote:
>>
>> Should I also add Intrinsic::annotation?
>
>
> Sure, sounds good (I see no reason for it to be omitted).
>
>
>  -Hal
>
>>
>> On Thu, Aug 31, 2017 at 11:52 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>>
>>> On 08/31/2017 03:37 PM, Ariel Ben-Yehuda wrote:
>>>>
>>>> But the code as present does an `argmemonly` check - I didn't put it
>>>> there.
>>>
>>>
>>> Indeed. This is wrong (the code here is assuming that loops can be
>>> infinite
>>> only if they essentially contain volatile writes representing I/O, and
>>> this
>>> is incorrect. Actual volatile accesses and atomics don't have this
>>> property). Nevertheless, this is a pre-existing problem, and fixing it is
>>> going to be a larger project. The only things in the current list that
>>> aren't read-none/argmemonly are ptr_annotation/var_annotation (and
>>> Intrinsic::annotation is missing it seems). Can you add those so we don't
>>> regress and then we can move forward with this.
>>>
>>> Thanks again,
>>> Hal
>>>
>>>
>>>> On Thu, Aug 31, 2017 at 11:33 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>>>>
>>>>> On 08/31/2017 03:22 PM, Ariel Ben-Yehuda wrote:
>>>>>>
>>>>>> Are not all the `isAssumeLikeIntrinsic` intrinsics are argmemonly?
>>>>>
>>>>>
>>>>> They might be, but the argmemonly check does not seem correct. I don't
>>>>> see
>>>>> what argmemonly has to do with the semantics for which you're checking
>>>>> (which, as explained in the comment above, is that the call much return
>>>>> (i.e. not exit the process or hang, etc.).
>>>>>
>>>>>    -Hal
>>>>>
>>>>>
>>>>>> On Thu, Aug 31, 2017 at 5:41 PM, Hal Finkel via Phabricator
>>>>>> <reviews at reviews.llvm.org> wrote:
>>>>>>>
>>>>>>> hfinkel added a comment.
>>>>>>>
>>>>>>> This looks good, however, you need to either:
>>>>>>>
>>>>>>> 1. Don't remove isAssumeLikeIntrinsic, but instead, update
>>>>>>> isGuaranteedToTransferExecutionToSuccessor to call it.
>>>>>>> 2. Update isGuaranteedToTransferExecutionToSuccessor to have the
>>>>>>> complete
>>>>>>> list from isAssumeLikeIntrinsic.
>>>>>>>
>>>>>>> Right now, isGuaranteedToTransferExecutionToSuccessor only checks:
>>>>>>>
>>>>>>>      return CS.onlyReadsMemory() || CS.onlyAccessesArgMemory() ||
>>>>>>>             match(I, m_Intrinsic<Intrinsic::assume>());
>>>>>>>
>>>>>>>
>>>>>>> https://reviews.llvm.org/D37215
>>>>>>>
>>>>>>>
>>>>>>>
>>>>> --
>>>>> Hal Finkel
>>>>> Lead, Compiler Technology and Programming Languages
>>>>> Leadership Computing Facility
>>>>> Argonne National Laboratory
>>>>>
>>> --
>>> Hal Finkel
>>> Lead, Compiler Technology and Programming Languages
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>>>
>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>