[PATCH] D38219: [GlobalISel][X86] Support G_LSHR/G_ASHR/G_SHL.

[Zvi Rackover via Phabricator via llvm-commits]

zvi added a comment.

LGTM with minor comments



================
Comment at: lib/Target/X86/X86InstructionSelector.cpp:1303
+                                         MachineFunction &MF) const {
+
+  assert((I.getOpcode() == TargetOpcode::G_SHL ||
----------------
Can you please add a comment here explaining how shift by constant shift-amount are handled? 


================
Comment at: lib/Target/X86/X86InstructionSelector.cpp:1319
+    unsigned OpSHL;
+  } OpTable[NumTypes] = {
+      {X86::CL, X86::SHR8rCL, X86::SAR8rCL, X86::SHL8rCL},     // i8
----------------
I don't think that NumTypes adds much here. You can just define OpTable[] = 


================
Comment at: lib/Target/X86/X86InstructionSelector.cpp:1330
+  unsigned TypeIndex;
+  if (DstTy.getSizeInBits() == 8)
+    TypeIndex = 0;
----------------
another option would be to search to avoid the chauned if's. Maybe something like:


```
find(OpTable, [DstTy] (const DhiftEntry& E) { return E.SizeInBits == DstTy.getSizeInBits(); })
```


https://reviews.llvm.org/D38219