[PATCH] D34769: [X86] X86::CMOV to Branch heuristic based optimization

[Zvi Rackover via Phabricator via llvm-commits]

zvi added a comment.

A few quick comments in the body of the implementation.

About the tests:
The added test are definitely interesting cases, but they are system tests as they check the full flow where we handle branches: CodegenPrepare, ISel, EarlyIfConversion (which is not enabled for X86), ...
Have you considered adding mit tests which will be considered as unit-tests specific to this pass?



================
Comment at: lib/Target/X86/X86CmovConversion.cpp:94
+  typedef SmallVector<CMOVGroup, 2> CMOVGroups;
+  CMOVGroups CmovInstGroups;
+
----------------
Please consider making the variable and type names consistent. Use either Cmov or CMOV.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:127
+  bool Changed = false;
+  MachineLoopInfo *MLI = &getAnalysis<MachineLoopInfo>();
+  const TargetSubtargetInfo &STI = MF.getSubtarget();
----------------
Define as a reference instead of a pointer.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:131
+  TII = STI.getInstrInfo();
+  TSchedModel.init(STI.getSchedModel(), &STI, TII);
+
----------------
Could we save the overhead of initializing the resource tables from one visited function to another? If two visited functions share the same subtarget, could we reuse the same schedule model configuration?


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:156
+  for (MachineBasicBlock &MBB : MF) {
+    CurrLoop = MLI->getLoopFor(&MBB);
+
----------------
This variable is not initialized in the c-tor, and IIUC will be assigned here for the first time. IMHO, it would be better to reduce its scope to this loop, and pass a reference in to collectCmovCandidates() and collectCmovCandidates().


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:170
+    Changed = true;
+    for (auto &Group : CmovInstGroups)
+      convertCmovInstsToBranches(Group);
----------------
To be consistent with collectCmovCandidates() and checkForProfitableCmovCandidates() i suggest you move this loop into convertCmovInstsToBranches().

Another option that IMHO is better:
Define CmovInstGroups in the scope of the MF iteration loop in this function and pass it by reference to the functions called in this loop. That will save the need to CmovInstGroups.clear() in numerous functions.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:204
+    // Indicator of a non CMOVrr instruction in the current processed range.
+    bool FoundNoneCMOVInst = false;
+    // Indicator for current processed CMOV-group if it should be skipped.
----------------
*FoundNonCMOVInst


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:211
+      // Check if we found a X86::CMOVrr instruction.
+      if (CC != X86::COND_INVALID && !I.mayLoad()) {
+        if (Group.empty()) {
----------------
If i did not miss it, can you please add a comment here or update the comments on the top why we skip cmovs with memory operands? My naive thought is that if we can speculate the load in the cmov, then we should also be able to put the load under a branch.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:226
+        continue;
+      }
+      // We found a non X86::CMOVrr instruction.
----------------
Would this allow making the code below simpler?
  
  else if (Group.empty()) continue;

It would probably be faster as the typical case is where no interesting instruction was encountered yet (or ever).

Another option is to search for the first interesting cmov instruction in the BB before entering this loop and and bail out if none found.


================
Comment at: lib/Target/X86/X86CmovConversion.cpp:243
+    // CMOV-group should not be skipped and add it as a CMOV-group-candidate.
+    if (!Group.empty()) {
+      if (!SkipGroup && !Group.empty())
----------------
Consider:

  if (Group.empty()) continue;



https://reviews.llvm.org/D34769