[EarlyCSE] Compare GEP instructions based on offset

[DianQK]

Closes #65763.
This will provide more opportunities for constant propagation for subsequent optimizations.

[nikic]

Please check the compile-time impact of this change. The impact was high when I tried something like this before, but maybe I made a mistake.

[DianQK]

Please check the compile-time impact of this change. The impact was high when I tried something like this before, but maybe I made a mistake.

Could you add https://github.com/DianQK/llvm-project to https://llvm-compile-time-tracker.com/? Thanks.

[nikic]

Please check the compile-time impact of this change. The impact was high when I tried something like this before, but maybe I made a mistake.

Could you add https://github.com/DianQK/llvm-project to https://llvm-compile-time-tracker.com/? Thanks.

I'll do this on Thursday. I'm currently on vacation and don't have the SSH key with me.

[nikic]

Please check the compile-time impact of this change. The impact was high when I tried something like this before, but maybe I made a mistake.

Could you add https://github.com/DianQK/llvm-project to https://llvm-compile-time-tracker.com/? Thanks.

I'll do this on Thursday. I'm currently on vacation and don't have the SSH key with me.

Done, results are here: http://llvm-compile-time-tracker.com/compare.php?from=ac1daad9bb4eb083df6b215c029816d3149e00d8&to=e079f3f9107f55e1595a47aafec5fe4e03277665&stat=instructions:u

[DianQK]

Please check the compile-time impact of this change. The impact was high when I tried something like this before, but maybe I made a mistake.

Could you add https://github.com/DianQK/llvm-project to https://llvm-compile-time-tracker.com/? Thanks.

I'll do this on Thursday. I'm currently on vacation and don't have the SSH key with me.

Done, results are here: http://llvm-compile-time-tracker.com/compare.php?from=ac1daad9bb4eb083df6b215c029816d3149e00d8&to=e079f3f9107f55e1595a47aafec5fe4e03277665&stat=instructions:u

Sad result. My initial guess was that in GEP 1 1 .... 1 1 a did redundant calculations.
But DianQK@f3fdea4 got worse results.
My other guess is that iteration itself is expensive. I'll try again. If that still doesn't work, I'd look into valgrind related tools.

[DianQK]

It's not ready for review yet. It may contain many nits.

Progress update:
Since lookups require frequent computation of constant offsets. GEP should not be considered as a SimpleValue.
I got better results than the last commit. I'll analyze it next with specific source code.

https://llvm-compile-time-tracker.com/compare.php?from=ac1daad9bb4eb083df6b215c029816d3149e00d8&to=31e2ec9d89aeded9ea5da822262449b0c4e8ab16&stat=instructions:u

[DianQK]

Using references works well!
https://llvm-compile-time-tracker.com/compare.php?from=ac1daad9bb4eb083df6b215c029816d3149e00d8&to=089349f9eb47c57cfa2049ce0af5c7dd079a8581&stat=instructions%3Au

I've also tried some approaches.
It should be useful to avoid offset calculations. I tried counting only with a GEP i8, but there is some extra cost to the determination of this one.
Simplifying the offset calculation is of some use, but I'm not sure it makes sense. Results are here https://llvm-compile-time-tracker.com/compare.php?from=ac1daad9bb4eb083df6b215c029816d3149e00d8&to=8ba478eb77c6dc09b87e9b511614e45e167be2b5&stat=instructions%3Au

[nikic]

Okay, those results look reasonable. It's somewhat unfortunate that this requires duplicating a bunch of code for the separate GEP handling. I'm wondering, would we be able to achieve similar results if we add a DenseMap<GetElementPtrInst *, APInt> cache for the offset but otherwise keep the previous code structure? Or would the extra hash lookup make this slow again?

[DianQK]

I'm wondering, would we be able to achieve similar results if we add a DenseMap<GetElementPtrInst *, APInt> cache for the offset but otherwise keep the previous code structure? Or would the extra hash lookup make this slow again?

I feel like I misunderstood. Do you mean putting the GEP back into SimpleValue? And then create a global (or EarlyCSE only) cache value?

[nikic]

I'm wondering, would we be able to achieve similar results if we add a DenseMap<GetElementPtrInst *, APInt> cache for the offset but otherwise keep the previous code structure? Or would the extra hash lookup make this slow again?

I feel like I misunderstood. Do you mean putting the GEP back into SimpleValue? And then create a global (or EarlyCSE only) cache value?

Yes (EarlyCSE only cache). This is just a suggestion for something to check, but I suspect the performance of that approach will not be good enough and we have to go with this version.

[DianQK]

I'm wondering, would we be able to achieve similar results if we add a DenseMap<GetElementPtrInst *, APInt> cache for the offset but otherwise keep the previous code structure? Or would the extra hash lookup make this slow again?

I feel like I misunderstood. Do you mean putting the GEP back into SimpleValue? And then create a global (or EarlyCSE only) cache value?

Yes (EarlyCSE only cache). This is just a suggestion for something to check, but I suspect the performance of that approach will not be good enough and we have to go with this version.

I did not try this approach. My understanding is that this only adds extra lookups if applied to a single EarlyCSE. Since each result needs to be calculated once. The current patch is the no-hash approach as I understand it.
This result may be invalid if shared in EarlyCSE.

I made a new attempt to save uint64_t instead of APInt.
This result looks pretty good. https://llvm-compile-time-tracker.com/compare.php?from=ac1daad9bb4eb083df6b215c029816d3149e00d8&to=82f1eca7c180b0dfe8af36e933670f938a49bab1&stat=instructions%3Au

[nikic]

LGTM