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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 2 inline comments as done.
fpetrogalli added inline comments.


================
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())
----------------
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.


================
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)>;
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83357