[PATCH] D75407: [InstSimplify] Constant fold icmp of gep

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added a comment.

In D75407#1901697 <https://reviews.llvm.org/D75407#1901697>, @efriedma wrote:

> > PS: Does someone know why we constant folding is separated in this way? I understand that we don't want to have ConstantExpr depend on TLI, but why can't it depend on DL?
>
> This is sort of historical; deep in LLVM's history, we allowed generating/optimizing modules without a datalayout.  This turned out to be mostly useless in practice, so we abandoned it many years ago.  The two separate constant folders is one of the leftover traces of that.
>
> It would be non-trivial to change at this point: in ConstantExpr::getICmp, there currently isn't any way to get a DataLayout.


Thanks for the historical context, that makes sense! Threading DL through 1.4k uses of ConstantExpr::get* indeed doesn't seem realistic at this point.



================
Comment at: lib/Analysis/InstructionSimplify.cpp:3487
+          return FoldedICmp;
+        return NewICmp;
       }
----------------
efriedma wrote:
> If we need to repeat this pattern in multiple places, probably makes sense to factor this out into a helper.
I was actually wondering whether we can make `ConstantFoldConstant()` simply always return non-null (either the original constant or a new one).

I initially thought that `ConstantFoldConstant()` follows the usual pattern where it returns non-null if and only if it actually folded something. However, this isn't actually the case: It returns null for non constexpr/constvector, but otherwise returns the original constant even if it didn't fold anything.

This seems like a not particularly useful in-between state and we could save this boilerplate (which is indeed repeated many times in the codebase) if we made ConstantFoldConstant() always return a constant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75407