[PATCH] D82261: [ValueTracking, BasicAA] Don't simplify instructions

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: fhahn, asbirlea.
Herald added subscribers: llvm-commits, kerbowa, dexonsmith, hiraditya, nhaehnle, jvesely.
Herald added a project: LLVM.
nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/promote-alloca-to-lds-select.ll:78
   ret void
 }
 
----------------
Let me explain what is going on here: We have selects on undefs involved here, which means that instruction simplification will simplify to the first select operand. If a real condition is used, then that of course doesn't happen. Because of that, this test is not testing anything useful right now.

In fact, I think this indicates that the current usage of SimplifyInstruction inside GetUnderlyingObject may be subtly unsound. GetUnderlyingObject will declare that the first select operand is the underlying object, but other code is permitted to later simplify that select to the second operand instead. In this case optimizations may have been performed based on an incorrect assumption. (While undef can be true or false, it cannot be both at the same time.)


`GetUnderlyingObject()` (and by required symmetry `DecomposeGEPExpression()`) will call SimplifyInstruction on the passed value if other checks fail. This simplification is very expensive, but has little effect in practice. This patch removes the SimplifyInstruction call, and replaces it with a check for single-argument phis (which can occur in canonical IR in LCSSA form), which is the only useful simplification case I was able to identify.

Compile-time numbers: http://llvm-compile-time-tracker.com/compare.php?from=be93ba1fd608cf9bef0a414c3193dff398c80c44&to=38bd3bc987b0c0f2e715859c78cd705e05689534&stat=instructions At O3 <https://reviews.llvm.org/owners/package/3/> the geomean improvement is -1.7%. The largest improvement is SPASS with ThinLTO at -6%.

In test-suite, I see only two tests with a hash difference and no code size difference (PAQ8p, Ptrdist), which indicates that the simplification only ends up being useful very rarely. (I would have liked to figure out which simplification is responsible here, but wasn't able to spot it looking at transformation logs.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82261

Files:
  llvm/lib/Analysis/BasicAliasAnalysis.cpp
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/test/CodeGen/AMDGPU/promote-alloca-to-lds-select.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D82261.272258.patch
Type: text/x-patch
Size: 4980 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200620/02155ef0/attachment-0001.bin>