[PATCH] D139645: [WebAssembly] Fold adds with global addresses into load offset

[Heejin Ahn via Phabricator via llvm-commits]

aheejin accepted this revision.
aheejin added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/test/CodeGen/WebAssembly/userstack.ll:336
+; global address, so we should fold the global address into the offset, but not
+; the frame offset.
 ; CHECK-LABEL: frame_offset_with_global_address
----------------
aheejin wrote:
> dschuff wrote:
> > dschuff wrote:
> > > luke wrote:
> > > > dschuff wrote:
> > > > > so the result of this is that the frame offset will end up in a const or add expression (consumed by the i32.load8_u), and the global address will be folded?
> > > > > Is this better than what we did before (i.e. the address in a const and the frame offset folded)?
> > > > In this case the frame offset wasn't being folded, although I'm not sure if that is intentional or not. This is the codegen before the patch:
> > > > 
> > > > ```
> > > > frame_offset_with_global_address:
> > > > 	.functype	frame_offset_with_global_address () -> (i32)
> > > > 	i32.const	$push0=, str
> > > > 	global.get	$push5=, __stack_pointer
> > > > 	i32.const	$push6=, 16
> > > > 	i32.sub 	$push9=, $pop5, $pop6
> > > > 	i32.const	$push7=, 12
> > > > 	i32.add 	$push8=, $pop9, $pop7
> > > > 	i32.add 	$push1=, $pop0, $pop8
> > > > 	i32.load8_u	$push2=, 0($pop1)
> > > > 	i32.const	$push3=, 67
> > > > 	i32.and 	$push4=, $pop2, $pop3
> > > > 	return	$pop4
> > > > ```
> > > > 
> > > > With the patch the global address gets folded in which saves a `const` and an `add` instruction:
> > > > 
> > > > ```
> > > > frame_offset_with_global_address:
> > > > 	.functype	frame_offset_with_global_address () -> (i32)
> > > > 	global.get	$push3=, __stack_pointer
> > > > 	i32.const	$push4=, 16
> > > > 	i32.sub 	$push7=, $pop3, $pop4
> > > > 	i32.const	$push5=, 12
> > > > 	i32.add 	$push6=, $pop7, $pop5
> > > > 	i32.load8_u	$push1=, str($pop6)
> > > > 	i32.const	$push0=, 67
> > > > 	i32.and 	$push2=, $pop1, $pop0
> > > > 	return	$pop2
> > > > ```
> > > > 
> > > > 
> > > Ah ok yeah this test was added in https://reviews.llvm.org/D90577 which fixed a bug in the case below the one you are modifying in this CL. So I guess what's happening now is that this IR is being ISel'ed differently and is ending up in the first case in eliminateFrameIndex instead of the second.
> > Maybe we could add to the comment something like "(this allows the global address to be relocated)"
> > 
> > Since this test doesn't cover that second case anymore, I wonder if we have any tests that do. I would assume that clause is still needed, maybe at least for when we aren't using DAG ISel...
> I commented the `if` added in D90577 out and ran this test, and this test still passes. When it was added that wouldn't have been the case, so I'm not sure what changed since then to make this pass. So what I'm saying is, this test is not covering the `if` in D90577 even now. 🤷🏻 Not sure what we should do. Delete the test and come up with a new test that covers the case? But that shouldn't be this CL's responsibility..
> 
> 
> @dschuff And I have another question. I am not very familiar with this prolog-epilog code..
> 1. What is the difference between what [[ https://github.com/llvm/llvm-project/blob/beb3fa2d2efbdc1fbedee2d5c587eae1364652d3/llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp#L71-L87 | this upper snippet ]] does and [[ https://github.com/llvm/llvm-project/blob/beb3fa2d2efbdc1fbedee2d5c587eae1364652d3/llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.cpp#L89-L113 | the lower snippet ]] does? Both seem to be about folding offsets.
> 
> 2. Why do we need to do custom folding here, and why isn't this taken care of the normal isel patterns? Is this for the case of fast isel?
By the way I don't think we need to hold up this CL because of this issue..?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139645