[PATCH] D94350: [GlobalISel] Combine (a[0]) | (a[1] << k1) | ...| (a[m] << kn) into a wide load

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

Thanks for tackling this. The logic seems sound to me, but I have some efficiency concerns. (Note, this is conjecture, I don't know if this is realistically a big problem)

G_OR trees merging data are probably fairly common. This algorithm tries to match an entire tree of them with their load/shifted operands starting from the root. The two issues I see is that:

1. If the actual legal load+OR tree is a subtree of the root node, we essentially do the work twice. If the combine does fire then this isn't a major issue since we do end up getting a nice code improvement from it.
2. If some part of the tree invalidates the combine, such as a leaf instruction having an invalid shift, then in the "worst-case" flattened tree list, we'll do (1/2)N^2 combine attempts until we finally exhaust all the G_OR instructions.

These aren't probably easy issues to solve without some intrusive changes. The former would need you to change the algorithm to go top-down and find the maximal mergeable subtree. The latter doesn't seem possible without also having some way to find which part of the tree is invalid.

I think 2 things would be good to know before going ahead with this patch. The first is compile time impacts if any on -O0 and maybe -Os too. The second is if you can add some debugging code to check how often we find tree candidates of say, more than 2 or 3 G_ORs, before *failing* to match.



================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3528
+
+  // We wil reuse the pointer from the load which ends up at byte offset 0. It
+  // may not use index 0.
----------------



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

https://reviews.llvm.org/D94350