[PATCH] D105889: [AArch64][SVE] Break false dependencies for inactive lanes of unary operations

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:457
     break;
+  case AArch64::ConstructiveUnaryPassthru:
   case AArch64::DestructiveBinaryImm:
----------------
This looks weird to me because there's nothing to say `DOPReg` is actually unique.  Here I think `ConstructiveUnaryPassthru` is special as for it's associated pseudo instructions the `passthru` operand is effectively irrelevant because all active lanes come from the data operand with the inactive lanes being undef or zero.

I imagine you are forcing true so that the new assert doesn't fire, but I think that new assert is also   irrelevant because we don't care about the other operands [1]. I don't have a problem forcing true but the assert just seems like dead code.

That said I'm then wondering what value there is referencing the passthrough value at all, but instead treating the data operand as `DOP` instead.  This seems weird but at the same time consistent with other instructions in that `DOP` contains real data for the active lanes.  If you did this then you will not need special handling within the `Create the destructive operation` block and in fact the `if (FalseZero)` part of that block will now be correct for if we even need zeroing pseudos for these unary ops.

What do you think?

I guess I'm suggesting:
`std::tie(PredIdx, DOPIdx, SrcIdx) = std::make_tuple(2, 3, 1);` or perhaps even `std::tie(PredIdx, DOPIdx, SrcIdx) = std::make_tuple(2, 3, 3);` which means you could use the same logic as `DestructiveBinaryImm` when regenerating the unary instruction. This might seem more palatable if changing the name for `ConstructiveUnaryPassthru` is agreeable.

[1] For what it worth I suspect there's a bug somewhere in this logic whereby `DstReg` is part of some of the `DOPRegIsUnique` computations but that's not your problem.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:39
 def DestructiveTernaryCommWithRev : DestructiveInstTypeEnum<8>;
+def ConstructiveUnaryPassthru     : DestructiveInstTypeEnum<9>;
 
----------------
I'll admit that I had no issues with this name until I wrote the large comment for `expand_DestructiveOp`. That said it has me wondering if these instructions can truely be considered constructive.  As I see it the unary passthrough instructions take two operands and munges them to produce a result.  This result is then used to overwrite the first operand (the one that is used when calculating the result for inactive lanes), which to my mind means that it is still a destructive operation. So `DestructiveUnaryPassthru` is a more accurate name.  What do you think?


================
Comment at: llvm/test/CodeGen/AArch64/sve2-unary-movprfx.ll:7
+;
+; SQABS
+;
----------------
I don't think you need to test all instructions but there should be a test for each new pattern. Can you ensure there is a test for one instruction within each modified group.  For example, I don't see any tests for the changes made to sve_int_un_pred_arit_1_fp, or is it the case there are existing tests for these?


================
Comment at: llvm/test/CodeGen/AArch64/sve2-unary-movprfx.ll:9
+;
+
+define <vscale x 16 x i8> @sqabs_i8_dupreg(<vscale x 16 x i8> %a) #0 {
----------------
For the first few tests can you add a short comment to describe what the test is verifying. I ask because I was initially confused with the output of `sqabs_i8_active` until I realised `_active` meant the predicate is all active and thus making the passthrough value dead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105889