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

[Amjad Aboud via Phabricator via llvm-commits]

aaboud marked 7 inline comments as done.
aaboud added a comment.

Thanks Zvi for the helpful comments.
I fixed most of them, and answered the others, see below.



================
Comment at: lib/Target/X86/X86CmovConversion.cpp:131
+  TII = STI.getInstrInfo();
+  TSchedModel.init(STI.getSchedModel(), &STI, TII);
+
----------------
zvi wrote:
> 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?
MachineCombiner, MachineLICM, and IfConverter initialize it in runOnMachineFunction, for every machine function.
Do you think that this function is too expensive?
Either way, it is out-of-scope for this patch.


================
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()) {
----------------
zvi wrote:
> 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.
The reason is implementation effort.
There is a "TODO" at line 191, so this can be added in future patch.
I do not see a need to add a comment, the "TODO" should be enough, do not you think so?


https://reviews.llvm.org/D34769