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

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added a comment.

In https://reviews.llvm.org/D45748#1111477, @asb wrote:

> 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.


Yes.  Are we still OK with committing this patch as is? I plan to add a test case that shows the peephole's in ability to handle a GlobalAddress uses at multiple blocks then commit this patch.

After that I'll look into doing all of this in a Machine Function Pass to handle all the corner cases. If it looks good we can revert this patch and keep the pass.

Does that sound like a good plan?


https://reviews.llvm.org/D45748