[PATCH] D50701: [MI] Change the array of `MachineMemOperand` pointers to be a generically extensible collection of extra info attached to a `MachineInstr`.

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

I'm happy with this, but I assume you want to respond to @bogner's comment.



================
Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:31
 #include "llvm/MC/MCInstrDesc.h"
+#include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/ArrayRecycler.h"
----------------
chandlerc wrote:
> rnk wrote:
> > We should be able to forward declare MCSymbol.
> No, and this is also why I can't move `ExtraInfo`'s definition as I had hoped.
> 
> Fundamentally, `PointerSumType` wants to check that the type parameters to it can be unioned into a sum type. To check that, it needs to know whether all of the tags will fit into the empty bits of the pointers. This requires the alignment of the pointer. The alignment requires the type to be complete.
> 
> We could delay this checking with some amount of work so that it is only triggered when the `PointerSumType` is *used*, but after trying several times to do that, I don't think I like the result at all.
> 
> First and foremost, the type *should* tell you, as early and as reliably as it can, that the parameters are valid. IMO, this should point the programmer at the *parameters*, not at some unrelated code. The declaration of the member is the best place to do this.
> 
> Second, the obvious way to defer this checking would cause unused parameters to *never be checked*. That seems ... really risky and bug prone to me.
> 
> Third, it seems reasonable for types with similar contracts (but perhaps slightly different implementation goals) to actually arrange their *storage* based on the alignment and available bits. It seems somewhat awkward to take advantage of the fact that `PointerSumType` *happens* to not change its layout / storage strategy based on the alignment just to defer checking of the validity of the template parameters given to it.
> 
> 
> I definitely don't think avoiding an include is worth the kind of usability hit this would incur.
> 
> I would be somewhat interested in avoiding this type cluttering up the public interface, but it at least needs to come before the *private* data member in which it is used. The correct way to clean this up, if folks want, is to actually the class contents holistically in a way more optimized for the reader of the API:
> 1. Private constants and type aliases used to make naming public types substantially simpler. (Optional and expected to be rare.)
> 2. Public constants and types, forward declared where the definition is large and unnecessary for public data members or member function declarations.
> 3. Public data members.
> 4. Public member functions, declaration only where the definition is large.
> 5. Private constants and types, and any public constant and type definitions declared (but not defined) in #1 and necessary for private data members or member function declarations.
> 6. Private data members.
> 7. Private member functions, declaration only where the definition is large.
> 8. (out-of-line from class body) Definitions of member functions declared above which are worth making inlinable and the definitions of constants and types only declared above necessary for those member function definitions. All of these ordered relative to each other based on the order in which they are declared within the class body.
> 9. (in the .cpp file) Remaining definitions, ordered relative to each other based on the order in which they are declared within the class body.
> 
> I much prefer this fairly strict ordering with very narrow and rare exceptions. I think it would also address some of your readability concerns here w/o forcing us to make declaration and definition able to be re-ordered.
> 
> 
> That said, this should be a follow-up change if we want to do it. I have tried to clean up the  comments and code a bit to make this slightly less of a readibility hit.
Fair enough. I'm probably less concerned about readability and more concerned about saving C++ template instantiation cruft in object files, but that's my twisted sense of priorities.


Repository:
  rL LLVM

https://reviews.llvm.org/D50701