[PATCH] D101005: [AArch64][GlobalISel] Simplify out of range rotate amount.

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.
Herald added a subscriber: tmatheson.

In D101005#2707670 <https://reviews.llvm.org/D101005#2707670>, @foad wrote:

> What's the point of this? Does it somehow end up generating better code?
>
> If it is useful, you could also do the same for FSHL and FSHR.

It allows us to select immediate forms of rotate instructions for AArch64.



================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3924
+  auto Bits = Builder.buildConstant(AmtTy, Bitsize);
+  ConstantFoldingMIRBuilder CFMIB(MI);
+  Amt = CFMIB.buildURem(AmtTy, MI.getOperand(2).getReg(), Bits).getReg(0);
----------------
foad wrote:
> arsenm wrote:
> > arsenm wrote:
> > > aemerson wrote:
> > > > arsenm wrote:
> > > > > aemerson wrote:
> > > > > > arsenm wrote:
> > > > > > > I think one off MIRBuilders should never be used
> > > > > > What do you suggest here instead?
> > > > > The combiner helper should have a universal builder
> > > > The universal builder is a CSE one, and I need a constant folding one here. Maybe CSE should do it too?
> > > I honestly don't see the point of having all of these MIRBuilder flavors. Why don't we have just one that does everything?
> > I thought the CSE builder did do constant folding. Also can't you just directly call the constant folding utility function?
> Yes the CSE builder does constant folding. I think ConstantFoldingMIRBuilder should be deleted. It's unused and I was told it was just an experiment.
Thanks, I hadn't realized since we didn't enable CSEInfo for this combiner pass so it wasn't working.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101005