[PATCH] D138904: [AArch64] Transform shift+and to shift+shift to select more shifted register

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:689-692
 bool AArch64DAGToDAGISel::SelectShiftedRegister(SDValue N, bool AllowROR,
                                                 SDValue &Reg, SDValue &Shift) {
+  if (SelectShiftedRegisterFromAnd(N, Reg, Shift))
+    return true;
----------------
bcl5980 wrote:
> mingmingl wrote:
> > Selecting `and (shl/srl/sra, x, c), mask` into `shl (srl/sra, x, c1), c2` (and folding shl) is simplifying one instruction away but could change the pipeline (latency and throughput as well) of the instruciton.
> > 
> > For example, arithmetic operations with shifted operand could use M pipeline [1] on neoverse n1, and M pipeline is used for all ALU operations with imm-shifted operand for cortex a57. 
> > 
> > I wonder if improvements are seen in some benchmarks from this patch?
> > 
> > [1] for neoverse n1,  M pipeline is used for {ADDS, SUBS}  with "Arithmetic, LSR/ASR/ROR shift or LSL shift > 4".
> I agree that it  may not get any timing improvement except lsl < 4. I also don't believe this patch can get improvement in any real benchmarks. Actually the current shifted register select function also does not consider for the thing you mentioned. If you really worry about that I can limit the lsl constant < 4.But personally I don't want to do that.
And for high-end CPU, it's really hard to model by just execute ports. 2 instructions will use 2 Rob entry, 2 physical registers, more front end decode, more code size. At least we should not consider these detail things when these pattern with the same latency and only some targets may have different throughput. If we really want to consider these detail micro architecture trade off, the better way is always do here and maybe split it in machine combiner that we already have detail schmodel.


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

https://reviews.llvm.org/D138904