andwar added a comment.
Cheers for working on this @kmclaughlin ! Have you considered adding an
interface for the new PM? You could check this for reference:
https://reviews.llvm.org/rGd6de5f12d485a85504bc99d384a85634574a27e2 (also
implements a FunctionPass).
================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:30
+
+#define DEBUG_TYPE "sve-intrinsicopts"
+
----------------
In AArch64TargetMachine.cpp it's:
```
static cl::opt<bool> EnableSVEIntrinsicOpts(
"aarch64-sve-intrinsic-opts", cl::Hidden,
cl::desc("Enable SVE intrinsic opts"),
cl::init(true));
```
so it probably make sense to use `see-intrinsic-opts` here as well?
================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:110
+ auto *PN = dyn_cast<PHINode>(X->getOperand(0));
+ if (!PN)
+ return false;
----------------
Given where this method is called, this is guaranteed to be always non-null.
Perhaps `assert` instead?
================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:120
+ if (!Reinterpret ||
+ RequiredType != Reinterpret->getArgOperand(0)->getType())
+ return false;
----------------
Isn't it guaranteed that `RequiredType ==
Reinterpret->getArgOperand(0)->getType()` is always true? I.e., `PN` and the
incoming values have identical type.
================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:125
+ // Create the new Phi
+ LLVMContext &C1 = PN->getContext();
+ IRBuilder<> Builder(C1);
----------------
[nit] Perhaps `Ctx` instead of `C1`?
================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:131
+
+ for (unsigned i = 0; i < PN->getNumIncomingValues(); i++) {
+ auto *Reinterpret = cast<Instruction>(PN->getIncomingValue(i));
----------------
`i` -> `I`
================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:187
+ // elide away both reinterprets if there are no other users of Y.
+ if (auto *Y = isReinterpretToSVBool(I->getArgOperand(0))) {
+ Value *SourceVal = Y->getArgOperand(0);
----------------
I'd be tempted to rewrite this to use early exit
(https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code):
```
auto *Y = isReinterpretToSVBool(I->getArgOperand(0));
if (nullptr == Y)
return false;
// The rest of the function
```
================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:224
+ bool Changed = false;
+ for (auto II = BB->begin(), IE = BB->end(); II != IE;) {
+ Instruction *I = &(*II);
----------------
1. Could this be a for-range loop instead?
2. This loop seems to be a perfect candidate for `make_early_inc_range`
(https://github.com/llvm/llvm-project/blob/172f1460ae05ab5c33c757142c8bdb10acfbdbe1/llvm/include/llvm/ADT/STLExtras.h#L499),
e.g.
```
for (Instruction &I : make_early_inc_range(BB))
Changed |= optimizeIntrinsic(&I);
```
================
Comment at: llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp:240
+ ReversePostOrderTraversal<BasicBlock *> RPOT(Root);
+ for (auto I = RPOT.begin(), E = RPOT.end(); I != E; ++I) {
+ Changed |= optimizeBlock(*I);
----------------
AFAIK, this iterates over BBs so `I` should be replaced with `BB`.
Also, perhaps `for (auto *BB : RPOT) {` instead?
================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsic-opts-ptest.ll:18
+; OPT-LABEL: ptest_any2
+; OPT: %out = call i1 @llvm.aarch64.sve.ptest.any.nxv16i1(<vscale x 16 x i1>
%1, <vscale x 16 x i1> %2)
+ %mask = tail call <vscale x 2 x i1> @llvm.aarch64.sve.ptrue.nxv2i1(i32 31)
----------------
What's `%1` and `%2`? Is it worth adding the calls that generated them in the
expected output?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76078/new/
https://reviews.llvm.org/D76078
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
