[PATCH] D5591: AArch64: Fold immediate into the immediate field of logical instructions

[Ahmed Bougacha via llvm-commits]

ab added a comment.

Some drive-by comments;  I haven't looked into the optimizeLogicalImm logic yet.



> TargetLowering.h:2252
> +  // Type of call-back function that does target-specific constant optimization.
> +  typedef std::function<bool(SDValue, const APInt &, TargetLoweringOpt &)>
> +      ConstOptFtorT;

Add more detail on when this is called and what the parameters are?

> TargetLowering.h:2278-2281
> +    ConstOptFtorT ConstOptFtor;
>  
> -    explicit TargetLoweringOpt(SelectionDAG &InDAG,
> -                               bool LT, bool LO) :
> -      DAG(InDAG), LegalTys(LT), LegalOps(LO) {}
> +    explicit TargetLoweringOpt(SelectionDAG &InDAG, bool LT, bool LO,
> +                               ConstOptFtorT Ftor = ConstOptFtorT()) :

I don't think this is the best location for this:  I'd rather have TargetLoweringOpt be "the result of an optimization", and TargetLowering be "how to do optimizations".

What do you think of going back to the TLI virtual hook, and fixing the other TLO methods to do the same:  https://reviews.llvm.org/differential/diff/73495/

> AArch64ISelLowering.cpp:745-746
> +                               unsigned NewOpc) {
> +  if (!EnableOptimizeLogicalImm)
> +    return false;
> +

Check this only once when initializing the std::function?  (or leave it here if you remove the std::function, I suppose)

> optimize-imm.ll:1
> +; RUN: llc -o - %s -mtriple=aarch64-unknown-unknown | FileCheck %s
> +

Triple can be simplified to: -mtriple=aarch64--

https://reviews.llvm.org/D5591