[PATCH] D128029: [AArch64] Add target feature "all"

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

No objections from me. I'm not an expert in this area so I've got a few questions/suggestions based on trying to understand the changes.



================
Comment at: llvm/test/TableGen/AsmPredicateCombining.td:53
 def AsmPred2 : Predicate<"Pred2">, AssemblerPredicate<(all_of AsmCond2a, AsmCond2b)>;
 def AsmPred3 : Predicate<"Pred3">, AssemblerPredicate<(any_of AsmCond3a, AsmCond3b)>;
 // MATCHER:      if (FB[arch::AsmCond1])
----------------
IIUC code has been added to be able to nest `any_of` and `all_of` could we add a test for that case.
 


================
Comment at: llvm/utils/TableGen/DecoderEmitter.cpp:1231
 
+bool FilterChooser::emitPredicateMatchAux(const Init &Val, bool Outer,
+                                          raw_ostream &OS) const {
----------------
Could you explain what Outer is precisely. To me the name implies it is meant to be true only when there is an expression with parens. For example any_of(x, y, z) when processing `y` assuming it isn't another `any_of` or `all_of` then I would expect Outer to be false (an inner if you like). It looks like Outer is set when processing an `all_of` or `any_of` with more than 1 parameter. If I'm right it might lead to more parentheses than we'd need, but this isn't a problem for correctness.


================
Comment at: llvm/utils/TableGen/DecoderEmitter.cpp:1252
+      for (auto *Arg : D->getArgs()) {
+        OS << LS;
+        if (emitPredicateMatchAux(*Arg, Outer, OS))
----------------
One thing I haven't been able to work out is whether it is possible for `Paren` to be true and `D->Args() > 0` so that we get `OS << '('` then `OS << LS` which I assume would end up as `(&&` or `(||` this doesn't look to be happening in the test cases, but I can't quite rule it out from the code here.


================
Comment at: llvm/utils/TableGen/SubtargetFeatureInfo.cpp:154
+    emitFeaturesAux(TargetName, *SFI.TheDef->getValueAsDag("AssemblerCondDag"),
+                    false, OS);
     OS << ")\n";
----------------
Worth adding a `false /* Outer */`?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128029