[PATCH] D142803: [LogicCombine 1/?] Implement a general way to simplify logical operations.

Sun Feb 26 20:56:35 PST 2023

bcl5980 added a comment.

In D142803#4150591 <https://reviews.llvm.org/D142803#4150591>, @spatel wrote:

> In D142803#4149087 <https://reviews.llvm.org/D142803#4149087>, @bcl5980 wrote:
>
>> In D142803#4148486 <https://reviews.llvm.org/D142803#4148486>, @nikic wrote:
>>
>>> I'm concerned about the caching here. It looks like you reuse one LogicCombiner instance for a basic block. However, isn't it possible for some of the instructions that have been inserted into LogicalOpNodes to be deleted, in which case the map may contain dangling pointers. If the pointer is reused by a newly allocated instruction, the cached information will be incorrect.
>>
>> The main reason for caching is saving compile time. The new patch will remove all the instructions already inserted into the caches and I think functional it works now.
>> @nikic @spatel if possible can we use the llvm-compile-time-track to test how much compile time increase if we enable the LogicCombiner for every single instruction?
>
> If this -- https://github.com/llvm/llvm-project/commit/efcf6c2b1f8490a9258d40abb90b21da60a15919 -- is the experiment that you wanted to try, it seems to have no significant difference:
> https://llvm-compile-time-tracker.com/compare.php?from=3592d05438acc1034905feff7ff555f4fd4c5774&to=efcf6c2b1f8490a9258d40abb90b21da60a15919&stat=instructions:u

For now I send 5 patches for this change. After more patches involved the compile time also increased. 
This is the result I enable LogicCombiner for every instruction after all my patches send to review:
http://llvm-compile-time-tracker.com/compare.php?from=4dd4eb939caef1138c655e22bb4adc8978f16427&to=8556a41a4ad4e8cf48bc316c9b5692b0de8e3d39&stat=instructions%3Au

And later I still need to figure out some headache things like trigger the undef unsafe pattern, restore "|" from the logical expression. So for now I still prefer to cache the result on basic block level.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142803/new/

https://reviews.llvm.org/D142803