[PATCH] D100691: [TableGen] Store predicates in PatternToMatch as ListInit *. Add string for HwModeFeatures

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:4370
-
-      // Add negations of the HM's predicates to the default predicate.
-      DefaultPred.emplace_back(Predicate(HM.Features, false));
----------------
kparzysz wrote:
> Is this part removed in your patch?  The "default" case could be vacuously true if there are no conditions attached to it, and it could be applied when a more specialized case should be applied instead.
> The "default" was originally invented for situations where the specific modes don't cover all possibilities when combined.  I guess in practice the "default" ends up being one of the explicitly defined modes, so this "accidental application" may not be happening.
I removed it because it wasn't doing what the comment said, the DefaultPred vector is only pushed to it's not used otherwise. Since it was incomplete, I didn't know how to update it.


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:1477
+    PredicateCheck += HwModeFeatures;
+    PredicateCheck += "\"))";
   }
----------------
kparzysz wrote:
> Paul-C-Anagnostopoulos wrote:
> > craig.topper wrote:
> > > Paul-C-Anagnostopoulos wrote:
> > > > Any reason you didn't combine the catenations with '+' ?
> > > Concatenating them would produce a temporary std::string that would need a heap allocation. That would then need to be copied into the SmallString. If the total string is small enough the SmallString won't heap allocate. If we copy each piece in separately we avoid heap allocations for temporaries.
> > Sounds like there's room for an optimization that looks at the complete sequence of catenations to decide whether there is room in the SmallString. But good to know.
> Isn't `Twine` specifically designed to optimize concatenations?
Yes, but Twine is used almost exclusively as an unnamed temporary object that is either passed to a function that takes a Twine argument or it is converted to a string before it goes out of scope. It has no operator+= so you can't incrementally update it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100691