On Jul 13, 2011, at 3:33 PM, David Greene wrote:
> Here is the patch to turn Inits into FoldingSetNodes. It depends on
> some mechanical patches to remove non-const iterators and to make Inits
> const, so it won't apply to trunk just yet.
>
> Is someone able to review it? Thanks!
Hi Dave,
What is the motivation for this? This patch looks mostly ok to me, with some specific requests below. Note that it is generally bad form to ask for a patch to be reviewed before the dependent patches are also available.
A pre-approved good way to get the mechanical part of this patch in is to *just* change "new whatever" to "whatever::get" where "whatever::get" is defined in terms of new. That will get the mechanical part of the patch out of the way and make it much easier to review the actual changes you're making.
That said, here's some thoughts:
>
> @@ -371,8 +372,40 @@ RecTy *llvm::resolveTypes(RecTy *T1, RecTy *T2) {
> // Initializer implementations
> //===----------------------------------------------------------------------===//
>
> +FoldingSet<Init> Init::UniqueInits;
> +BumpPtrAllocator Init::InitAllocator;
Please don't introduce more global variables with static ctors into things. tblgen is not exactly a paragon of cleanliness, but it would be nice to not make it any worse. If there is no good alternative, since this is "just tblgen" it is ok. In that case, just make it a static global in the .cpp file instead of exposing it in the header as a static class member.
> +
> void Init::dump() const { return print(errs()); }
>
> +const UnsetInit *UnsetInit::get() {
> + FoldingSetNodeID ID;
> + ID.AddInteger(initUnset);
There is only one of these, why put it into a folding set?
> +const BitInit *BitInit::get(bool V) {
> + FoldingSetNodeID ID;
> + ID.AddInteger(initBit);
> + ID.AddBoolean(V);
Likewise, there are only two of these.
> +const IntInit *IntInit::get(int64_t V) {
> + FoldingSetNodeID ID;
> + ID.AddInteger(initInt);
> + ID.AddInteger(V);
Why use a FoldingSet for this instead of a DenseMap? It seems like completely overkill.
> +const StringInit *StringInit::get(const std::string &V) {
> + FoldingSetNodeID ID;
> + ID.AddInteger(initString);
> + ID.AddString(V);
Similarly, should be a stringmap.
> +const CodeInit *CodeInit::get(const std::string &V) {
> + FoldingSetNodeID ID;
> + ID.AddInteger(initCode);
> + ID.AddString(V);
again.
> +const ListInit *ListInit::get(std::vector<const Init *> &Vs, RecTy *EltTy) {
> + FoldingSetNodeID ID;
> + ID.AddInteger(initList);
> + ID.AddString(EltTy->getAsString());
Ok, foldingset makes more sense for this.
> @@ -530,6 +629,23 @@ const Init *OpInit::resolveListElementReference(Record &R, const
> +const UnOpInit *UnOpInit::get(UnaryOp opc, const Init *lhs, RecTy *Type) {
> + FoldingSetNodeID ID;
> + ID.AddInteger(initUnOp);
> + ID.AddInteger(opc);
> + ID.AddString(Type->getAsString());
> + ID.AddPointer(lhs);
DenseMap makes sense here.
> +const BinOpInit *BinOpInit::get(BinaryOp opc, const Init *lhs,
> + const Init *rhs, RecTy *Type) {
and here.
>
> +const TernOpInit *TernOpInit::get(TernaryOp opc, const Init *lhs,
> + const Init *mhs, const Init *rhs,
> + RecTy *Type) {
and here.
etc.
-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110713/1ede43c8/attachment.html>