[PATCH] D128029: [AArch64] Add target feature "all"
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 29 10:28:05 PDT 2022
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
More information about the llvm-commits
mailing list