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

[Ariel Ben-Yehuda via llvm-commits]

Should I also add Intrinsic::annotation?

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
>