Page MenuHomePhabricator

[EarlyCSE] make abs recognization not depend on instcombine abs canonicalize
ClosedPublic

Authored by shchenz on Nov 3 2020, 9:35 PM.

Details

Summary

EarlyCSE pass runs very early at the pipeline of opt binary, before inst combine pass.

For now EarlyCSE pass only recognizes the abs pattern after the canonicalize of abs in instcombine pass. This will make EarlyCSE miss some opportunities.

Also only recognizing some abs pattern will make equal instructions have different hash value. For example, see https://bugs.llvm.org/show_bug.cgi?id=48058

  %neg = sub i8 0, %a
  %cmp1 = icmp slt i8 %a, 0
  %cmp2 = icmp sge i8 %a, 0 
  %m1 = select i1 %cmp1, i8 %a, i8 %neg
  %m2 = select i1 %cmp2, i8 %neg, i8 %a
  %r = xor i8 %m2, %m1
  ret i8 %r
`

%m1 and %m2 are recognized as same instructions in isEqualImpl because of inverse predicate and same operands for these two select.

But in getHashValueImpl, %m1 is recognized as NABS because it is already in canonicalize form of abs. But %m2 is not in canonicalize form of abs, so it is not recognized as NABS.

Diff Detail

Event Timeline

shchenz created this revision.Nov 3 2020, 9:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2020, 9:35 PM
shchenz requested review of this revision.Nov 3 2020, 9:35 PM
spatel added a comment.Nov 8 2020, 7:19 AM

The medium-term solution (hopefully not too far away now) is to canonicalize to intrinsics - D87188. Once we get D90554 in, we can try that canonicalization again.

So I'm not sure that we want to add to the complexity of early-cse to handle this right now. What if we just delete the code that matches to SPF_ABS / SPF_NABS. I'm not seeing any regression test failures with that, and it avoids the bug?

The medium-term solution (hopefully not too far away now) is to canonicalize to intrinsics - D87188. Once we get D90554 in, we can try that canonicalization again.

So I'm not sure that we want to add to the complexity of early-cse to handle this right now. What if we just delete the code that matches to SPF_ABS / SPF_NABS. I'm not seeing any regression test failures with that, and it avoids the bug?

Thanks for your comments. I am fine to delete the SPF_ABS/SPF_NABS related codes in earlycse pass. In current opt pipeline, passes between the first earlycse and the first instcombine should not be sensitive to earlycse's result or will be rerun after instcombine? Please let me know if you need me to delete them.

spatel added a comment.Nov 9 2020, 5:28 AM

The medium-term solution (hopefully not too far away now) is to canonicalize to intrinsics - D87188. Once we get D90554 in, we can try that canonicalization again.

So I'm not sure that we want to add to the complexity of early-cse to handle this right now. What if we just delete the code that matches to SPF_ABS / SPF_NABS. I'm not seeing any regression test failures with that, and it avoids the bug?

Thanks for your comments. I am fine to delete the SPF_ABS/SPF_NABS related codes in earlycse pass. In current opt pipeline, passes between the first earlycse and the first instcombine should not be sensitive to earlycse's result or will be rerun after instcombine? Please let me know if you need me to delete them.

We run earlycse at least once after instcombine in the typical optimization pipeline, so I don't think there is much chance for regression.

Can you update this patch to remove the SPF_ABS/SPF_NABS block and keep the new test to verify that we are not failing the hash matching?

shchenz updated this revision to Diff 304035.Nov 9 2020, 8:36 PM

fix according to comments:
1: delete abs handling in earlycse pass.

@spatel updated. I guess we need to add abs handling later in earlycse pass after D87188 is committed?

@spatel updated. I guess we need to add abs handling later in earlycse pass after D87188 is committed?

I am not seeing what we would need to do with this pass - if we canonicalize to the intrinsic form, then CSE just becomes a simple match of the intrinsic arguments.
Do you have an example of something that would fail to optimize or hash properly?

I am thinking llvm.abs(A) and llvm.abs(-A) can not be optimized in current earlyCSE phase? There are logics for commutative intrinsics and intrinsics with same operands in EarlyCSE pass. Am I missing something?

I am thinking llvm.abs(A) and llvm.abs(-A) can not be optimized in current earlyCSE phase? There are logics for commutative intrinsics and intrinsics with same operands in EarlyCSE pass. Am I missing something?

Ah, I know your ideas ^-^. Instcombine should canonicalize llvm.abs(-A) to llvm.abs(A). So after instcombine, in earlycse, we will not see llvm.abs(-A) right? I don't have other concerns. Thanks.

spatel accepted this revision.Nov 10 2020, 6:23 AM

I am thinking llvm.abs(A) and llvm.abs(-A) can not be optimized in current earlyCSE phase? There are logics for commutative intrinsics and intrinsics with same operands in EarlyCSE pass. Am I missing something?

Ah, I know your ideas ^-^. Instcombine should canonicalize llvm.abs(-A) to llvm.abs(A). So after instcombine, in earlycse, we will not see llvm.abs(-A) right? I don't have other concerns. Thanks.

Right - I do not think we should put any extra pattern matching logic into this pass unless it is very simple. Everything else is dangerous - as this bug/patch (and a few before it) have already shown. :)
LGTM.

This revision is now accepted and ready to land.Nov 10 2020, 6:23 AM

I am thinking llvm.abs(A) and llvm.abs(-A) can not be optimized in current earlyCSE phase? There are logics for commutative intrinsics and intrinsics with same operands in EarlyCSE pass. Am I missing something?

Ah, I know your ideas ^-^. Instcombine should canonicalize llvm.abs(-A) to llvm.abs(A). So after instcombine, in earlycse, we will not see llvm.abs(-A) right? I don't have other concerns. Thanks.

Right - I do not think we should put any extra pattern matching logic into this pass unless it is very simple. Everything else is dangerous - as this bug/patch (and a few before it) have already shown. :)
LGTM.

Thank you!

This revision was landed with ongoing or failed builds.Nov 10 2020, 6:12 PM
This revision was automatically updated to reflect the committed changes.