[PATCH] D54698: [SelectionDAG] Initial support for FSHL/FSHR funnel shift opcodes (PR39467)

Mon Dec 3 07:51:25 PST 2018

RKSimon marked an inline comment as done.
RKSimon added a comment.

Any more comments?

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7001
+  unsigned RotOpc = IsFSHL ? ISD::ROTL : ISD::ROTR;
+  if (N0 == N1 && hasOperation(RotOpc, VT))
+    return DAG.getNode(RotOpc, SDLoc(N), VT, N0, N2);
----------------
nikic wrote:
> RKSimon wrote:
> > nikic wrote:
> > > Similar to what the DAG builder code does, would it make sense to also check for the reverse direction rotate here?
> > Makes sense - always (and a SUB) or just for constant shift amounts?
> Hard to say. If say FSHL is legal, but ROTL is not, but ROTR is, then it probably does not make sense to convert a legal FSHL to SUB, ROTR. Though that situation seems rather contrived.
> 
> Maybe it's best to wait with this transformation until it becomes necessary (if/when FSH is built directly.)
I've added a TODO comment for now 

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54698