[PATCH] D11363: Allow merging of immediates within a basic block for code size savings and reduced footprint.

[Quentin Colombet]

qcolombet added a comment.

Hi Zia,

> ConstantHoisting.cpp can make the constants more opaque, preventing isel from pulling them, back in, but this is phase dependent. How about constants introduced after hoisting? DAG creation will always pull out constants, and it seems more natural to properly select the right instructions after this transformation, given our heuristics.


Agreed, however I do not believe any of the test cases in that patch fall into that case. I.e., I’d rather have an actual motivating example not to use ConstantHoisting than adding extra logic.

> Perhaps more importantly : Pulling out all common constants in ConstantHoisting.cpp might affect other downstream target-independent optimizations on the IL that look for constants as part of the instructions. It's easier to optimize with subsumed immediates at the IL level. My changes may have the same effect in the CG, but they occur later in isel, and are a little more natural for something that's more target-specific (i.e. dealing with instruction encoding).


This is a phase ordering problem and if I remember correctly ConstantHoisting runs very late and already produces “target-dependent” code, since the constants to hoist are chosen given some target dependent cost model. In other words, this shouldn’t be a problem with the current setting.

I am aware that pushing to reuse ConstantHoist may not be the ultimate solution, but I miss compelling test cases/arguments for not using/improving it right now. Put differently, I’d like to see whether or not we can improve the generic framework for all targets to benefit from it, before exploring target specific solutions.

Cheers,
-Quentin


http://reviews.llvm.org/D11363