[PATCH] D83357: [llvm][sve] Reg + Imm addressing mode for ld1ro.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12308
+  if (VT == MVT::nxv8bf16 &&
+      !static_cast<const AArch64Subtarget &>(DAG.getSubtarget()).hasBF16())
+    return SDValue();
----------------
I think this should be `cast`, not `static_cast`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12229
   EVT VT = N->getValueType(0);
+  if (VT == MVT::nxv8bf16 &&
+      !static_cast<const AArch64Subtarget &>(DAG.getSubtarget()).hasBF16())
----------------
fpetrogalli wrote:
> sdesmalen wrote:
> > This change doesn't seem to be tested. Does this belong in a separate patch?
> This makes sure that the `bfloat` case in the test file will work only if the `+bf16` is added to the target feature. If I remove this, the `bfloat` case will lower even without the `+bf16` target feature. 
> 
> To properly test this, I shall create an `llc` test that fails with a when `+bf16` is not listed as a target feature and that catches the "missing patters" debug message from the failure. I don't think we have tests like these, but let me know if this is something you want me to add one.
> 
> FWIW, this "early exit if lowering bfloat when not targeting bfloat" code is used also in other places, like `performST1Combine`. It is needed because we have encoded only integer patterns in the tablegen files, and we lower FP values by bitcasting them to integer values.
Ah okay, so it is kind of tested, i.e. when removing the `+bf16` attribute the test would now fail, where it didn't previously. I don't think a special negative test is needed though.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:7734
+  let AddedComplexity = 2 in {
+  // Reg + Imm addressing mode
+    def : Pat<(Ty (Ld1ro (PredTy PPR3bAny:$Pg), (add GPR64:$base, (i64 simm4s32:$imm)))),
----------------
nit: odd indentation.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:7725
+  // Reg + Imm addressing mode
+    def : Pat<(Ty (Ld1ro PPR3bAny:$gp, (add GPR64:$base, (i64 simm4s32:$imm)))),
+              (!cast<Instruction>(NAME) $gp, $base, simm4s32:$imm)>;
----------------
fpetrogalli wrote:
> sdesmalen wrote:
> > A few questions:
> > - Should this pattern also use `(PredTy PPR3bAny:$gp)`?
> > - Why `2` in `AddedComplexity = 2` ?
> > - Is AddedComplexity needed at all? (it already looks more 'complex' than the other pattern)
> > 
> > nit: i don't know why `gp` is used here instead of `Pg`, which is used in the rest of file.
> > Should this pattern also use (PredTy PPR3bAny:$gp)
> 
> Done.
> 
> > Why 2 in AddedComplexity = 2 ?
> 
> I don't know, but neither no added complexity or AddedComplexity = 1 work. In both cases, the reg+reg addressing mode specified in the multicalss `sve_mem_ldor_ss` wins. `AddedComplexity = 2` is the first non-zero value that makes it work...
> 
> > nit: i don't know why gp is used here instead of Pg, which is used in the rest of file.
> 
> I made the class use `$Pg`. I guess I got `$gp` by copy-paste from other places (I can see few in this file).
Okay thanks for clarifying and fixing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83357