[PATCH] D86786: GlobalISel: Combine out redundant sext_inreg

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.
Herald added a subscriber: danielkiss.

In D86786#2244835 <https://reviews.llvm.org/D86786#2244835>, @aemerson wrote:

> In D86786#2244826 <https://reviews.llvm.org/D86786#2244826>, @arsenm wrote:
>
>> In D86786#2244814 <https://reviews.llvm.org/D86786#2244814>, @aemerson wrote:
>>
>>> computeNumSignBits() does not mean that the value was sign extended. It only returns you the number of top-most bits that are known to be the same. As a result, this also ends up matching G_ZEXTLOAD which is a bug I hit in an earlier attempt at this.
>>
>> It is correct to match zextload though, you just know 1 fewer bit vs. sextload. It doesn't matter what the source is. This is exactly what DAGCombiner does: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L11054
>
> Ah, I think the bug may have been an off by one issue then. So this combine can be deleted then: 64eb3a4915f0 <https://reviews.llvm.org/rG64eb3a4915f00cca9af4c305a9ff36209003cd7b>?



In D86786#2244835 <https://reviews.llvm.org/D86786#2244835>, @aemerson wrote:

> In D86786#2244826 <https://reviews.llvm.org/D86786#2244826>, @arsenm wrote:
>
>> In D86786#2244814 <https://reviews.llvm.org/D86786#2244814>, @aemerson wrote:
>>
>>> computeNumSignBits() does not mean that the value was sign extended. It only returns you the number of top-most bits that are known to be the same. As a result, this also ends up matching G_ZEXTLOAD which is a bug I hit in an earlier attempt at this.
>>
>> It is correct to match zextload though, you just know 1 fewer bit vs. sextload. It doesn't matter what the source is. This is exactly what DAGCombiner does: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L11054
>
> Ah, I think the bug may have been an off by one issue then. So this combine can be deleted then: 64eb3a4915f0 <https://reviews.llvm.org/rG64eb3a4915f00cca9af4c305a9ff36209003cd7b>?

Yes, I think so. I'll try to delete those after D86787 <https://reviews.llvm.org/D86787>


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

https://reviews.llvm.org/D86786