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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/MC/AArch64/directive-arch_extension.s:99
+
+.arch_extension all
+// ERR: :[[#@LINE-1]]:17: error: unknown architectural extension: all
----------------
lenary wrote:
> Can you move this test into `directive-arch_extension-negative.s`, where we already expect the directive to fail and emit errors?
Done. For many tests, I think using one file for both positive and negative tests may be clearer. Since `directive-arch_extension-negative.s` exists, I'll not change it in this patch :-)


================
Comment at: llvm/utils/TableGen/DecoderEmitter.cpp:1231
 
+bool FilterChooser::emitPredicateMatchAux(const Init &Val, bool Outer,
+                                          raw_ostream &OS) const {
----------------
peter.smith wrote:
> 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.
Thanks for the comments. Added a comment.

`Outer` is simplified from a more complex recursive expression printer.

```
a * (b + c)
    ------- current recursive call
```
For a recursive call where OuterPrecedence indicates `*`, if the inner expression uses an operator (e.g. `+`) which binds less than the outer operator, a pair of `()` is needed.

In our case, for the following expression
```
a && (b && c)
     ________
```
we print a pair of superfluous parentheses. But for most cases, the new algorithm prints fewer `()` than before.


================
Comment at: llvm/utils/TableGen/DecoderEmitter.cpp:1252
+      for (auto *Arg : D->getArgs()) {
+        OS << LS;
+        if (emitPredicateMatchAux(*Arg, Outer, OS))
----------------
peter.smith wrote:
> 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.
`ListSeparator` does not print a delimiter for the first `<<` call. `(&&` or `(||` cannot happen.


================
Comment at: llvm/utils/TableGen/SubtargetFeatureInfo.cpp:154
+    emitFeaturesAux(TargetName, *SFI.TheDef->getValueAsDag("AssemblerCondDag"),
+                    false, OS);
     OS << ")\n";
----------------
peter.smith wrote:
> Worth adding a `false /* Outer */`?  
I pick `/*ParenIfBinOp=*/false`. This style is allowed by both clang-format and clang-tidy's bugprone-argument-comment check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128029