[PATCH] D119013: [ArgPromotion][AMDGPU] New MSSA-based function argument promotion pass with input/output argument support

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

>From a cursory look at the implementation, does this handle unwinding properly?

  store i32 0, ptr %arg
  call void @may_unwind() readnone
  store i32 1, ptr %arg

I believe promotion is not possible in this case, because there's no good way to provide the new value on the unwind path to the caller.



================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:711
+void ArgumentPromoter::promoteInOutArg(ArgPromotionInfo &ArgInfo,
+                                       RetValuesMap &RetValues) {
+  SmallDenseMap<BasicBlock *, SmallVector<Instruction *, 4>, 16> MemInsts;
----------------
vpykhtin wrote:
> nikic wrote:
> > For the actual promotion, have you considered making use of PromoteMemToReg? Basically, replace the old argument with an alloca that is stored on entry and read on exit, and then run mem2reg on that alloca?
> > 
> > ```
> > define i32 @test(i32 %arg) {
> >   %old.arg = alloca i32
> >   store i32, i32* %old.arg
> >   // Code uses old.arg
> >   %ret = load i32, i32* %old.arg
> >   ret i32 %ret
> > }
> > ```
> I think this is a good idea to reuse the working code, however I would rather make some sort of refactoring on it than trying to please it :) There is similar code in LICM promoteLoopAccessesToScalars, so it seems we would benefit of such mem2reg framework.
LICM uses the LoadAndStorePromoter utility instead -- I'm not really familiar with the differences between these approaches. From what I can tell, LoadAndStorePromoter is SSAUpdater-based and can work without DomTree, while PromoteMemToReg is more similar to the code you use here, in that it uses IDF.


================
Comment at: llvm/test/Transforms/ArgumentPromotion/inoutargs2.ll:5
+; REF-LABEL: define internal void @no_ret_blocks() #0 {
+; REF-NEXT: unreachable
+define internal void @no_ret_blocks() #0 {
----------------
What's up with these REF labels?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119013