[PATCH] D45748: [RISCV] Add peepholes for Global Address lowering patterns

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In https://reviews.llvm.org/D45748#1110695, @sabuasal wrote:

> In https://reviews.llvm.org/D45748#1110644, @asb wrote:
>
> > I'm happy to land this patch as-is. It generates good code in a wide variety of cases (more so than many other backends). But please add the above example to your test file with a note about the missed optimisation opportunity.
>
>
> The example is actually already added to the test (define dso_local i32 @load_half() nounwind).


Sorry, I was focusing on the diff and had forgotten you already added this in the previous patch. Thanks!

> On the other hand, after some chat with @efriedma  I am starting to think that this whole thing could be better handled with a machine function Pass that runs after MachineCSE.  Your point in your original longer example (and the example I have in the test case called control_flow) shows that we are getting different notes for the lowered global address (just run llc with --stop-before=machine--cse), hasUseOnce always considers the uses within the basic block you are on, not the whole function (which makes sense). The reason these two examples worked (and by worked I mean NOT having the offset merged back into the global address lowering) is because the ADDI generated for the offset was folded into the Load\STore and my peephole found no Tail to kick in. If we add a MachineFunctionPasss we we will have:

I agree with your analysis. It's conceivable you could have an IR pass that tries to improve codegen, but it would have to operate based on assumption about how global lowering would work. I think as you suggest, a machine function pass is promising.

So would the plan be to always generate offset separate from the global (as is current upstream behaviour after your recent patch), and to opportunistically merge the offset in the case the the global base only has a single reference, or (optionally, probably less common) every reference using that base has the same offset.


https://reviews.llvm.org/D45748