[PATCH] D15730: [MachineLICM] Fix handling of memoperands

[Andrew Trick via llvm-commits]

atrick added a comment.

In http://reviews.llvm.org/D15730#316212, @reames wrote:

> Original patch submitted, but let's keep the discussion going. Once we settle on what should be, I'm going to prepare a follow on patch to clarify/enforce invariants.
>
> > 
>
> > 
>
> > - Going from zero to non-zero memoperands would indeed be an error.
>
>
> I'm not sure this will work in practice.  I haven't tried yet, but I believe we're likely to use zero as a starting state and then add all the needed operands.  We do need a conservative state, but I suspect that will have to be a separate explicit poison state.
>
> > 
>
> > 
>
> > - Casual pass writers often forget to propagate memoperands entirely (naïve sloppiness is ok), but dropping only some of them and not all of them would be incorrect (conscious sloppiness is an error).
>
> > - addMemOperand is supposed to be an "internal" API, not called directly from passes.
>
>
> This doesn't look to be actual true.  Or at least, "internal" means a lot more code than I'd think it should...


"Internal" was a bad choice of words. I mean that it is designed as a utility to use within code that already knows how to correctly build an instruction or merge memoperands. addMemOperand seems mostly like a convenience for constructing stack load/stores. It turns out we do this a lot. There's no utility to encapsulate that operation, and there's no way for addOperands to know this is a "new" instruction.

In fact, I'm not aware of any reason to add memoperands at all outside of building a new instruction. When merging instructions  you're likely to just create a new one, not modify an old one.

Maybe your fear is that an instruction is built up in stages. Initially something indicates an unknown memory location so we zero memoperands, then later we acquire a memoperand. That would be a bug. I agree it would be better to insert a PseudoSourceValue placeholder when we see an unknown location.

> > - Merging memory accesses cannot be done naïvely.

> 

> 

> Can you describe how it can be done at all?  I don't know today.


I should restate that: merging load/stores can be done naïvely by dropping all memoperands. The danger would be in referring to the memoperands list from only one of the original load/stores without merging them into a new memoperands list.

AArch64 uses a concatenateMemOperands utility.

I meant to contrast this with splitting a load/store, which could either very conservatively drop memoperands or much less conservatively refer to the original memoperands list in both places.

> > - We should work hard to avoid empty memoperands, and to that end we could introduce PseudoSourceValues with specific meanings. I certainly don't think that overflowing memoperands should result in a zero memoperands state, which as Sanjoy pointed out is more error prone.

> 

> 

> How do we avoid this?  Do we introduce more abstract/less precise PSVs?  I'm open to ideas here; I don't really claim to understand what all we're using this for.


That was just a suggestion for improvement. I don't have a better idea for representing an unknown memory location than with a new kind of PSV.

Andy


Repository:
  rL LLVM

http://reviews.llvm.org/D15730