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

[Hal Finkel via llvm-commits]

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