[PATCH] D37289: [X86] Speculatively load operands of select instruction

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:16266
+              GEPFalse->hasAllConstantIndices())
+            LLD->dump();
+          return false;
----------------
Do you need this dump()? It will break compilation of release mode.
Either remove it or use it under DEBUG() macro.


================
Comment at: lib/Target/X86/X86SpeculateSelectLoad.cpp:25
+#include "llvm/Support/Debug.h"
+#include <deque>
+
----------------
Maybe, I missed that, but why do you need this include for?


================
Comment at: lib/Target/X86/X86SpeculateSelectLoad.cpp:98
+    // Get the indices of the elements in the struct
+
+    ConstantInt *Idx1 = dyn_cast<ConstantInt>(GEPIT->getOperand(2));
----------------
Remove this empty line.


================
Comment at: lib/Target/X86/X86SpeculateSelectLoad.cpp:120
+      LoadInst *LF = Builder.CreateAlignedLoad(GEPIF, LI->getAlignment());
+      SelectInst *NewSI =
+          cast<SelectInst>(Builder.CreateSelect(SI->getCondition(), LT, LF));
----------------
NewSI can be of type Value*, and would not need the cast to SelectInst.
Notice that the only use for NewSI, is "replaceAllUsersWith()" which takes Value* as argument.


================
Comment at: test/CodeGen/X86/speculate-select-load.ll:28
+; Function Attrs: norecurse nounwind readonly uwtable
+define i64 @no_spec_load(i32 %x, i64 %A, %struct.S* nocapture readonly %B) local_unnamed_addr #0 {
+entry:
----------------
Can you add a comment explaining this case (and the one above).
Something like: Selecting between address of/pointer to two members of same structure, with offset bigger than cache line (64 bytes) between them. Thus, do not load speculatively.


Repository:
  rL LLVM

https://reviews.llvm.org/D37289