[PATCH] D75807: [InstCombine] fold gep-of-select-of-constants (PR45084)

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 6 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1816
+                                               m_Constant(FalseC))))
+    return nullptr;
+
----------------
nikic wrote:
> Should the select be limited to one use? I'm not sure whether this transform is favorable if we need to keep both selects. In either case, this should be tested.
Unless there's evidence that the select-of-constants form is less favorable, we want to allow the transform because it eliminates a use of the existing select. I'll add a test.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1822
+  TrueC = ConstantExpr::getGetElementPtr(GEPSrcTy, TrueC, IndexC);
+  FalseC = ConstantExpr::getGetElementPtr(GEPSrcTy, FalseC, IndexC);
+  return SelectInst::Create(Cond, TrueC, FalseC);
----------------
nikic wrote:
> It is better to construct these GEPs via IRBuilder, so the resulting constants get folded right away (GEPs are target-dependent).
> 
> Also, aren't we losing inbounds here?
Can you describe the benefit of IRBuilder vs. ConstantExpr::get()? I'm not seeing any difference in the debug output for the examples that I have. 

I'm happy to change it (but we should probably change a bunch of existing similar code too?).

I'm not clear on the inbounds rules here (it's a constant, so is it not required to be inbounds?). But yes, I'll fix this to inherit from the existing instruction.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1823
+  FalseC = ConstantExpr::getGetElementPtr(GEPSrcTy, FalseC, IndexC);
+  return SelectInst::Create(Cond, TrueC, FalseC);
+}
----------------
nikic wrote:
> We can preserve branch weight metadata here.
Yes, good catch - I'll add to the test.


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

https://reviews.llvm.org/D75807