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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Zia,

Thanks for sharing your point of view on that.

My position can be summarized like this:
I do not want to prevent you to go the ISel path, but if you do, please make sure to expose at lest a test case that couldn’t be tackled by ConstantHoisting, i.e., a case where constants are exposed at ISel time.
Whatever we do to make ISel clever will not scale to function level scope, so why not invest directly into ConstantHoisting? I reckon this may be a bigger commitment as ConstantHoisting may, like you said, be clumsy.

That being said, I do believe ConstantHoisting is a bit too high level for this kind of optimization and we do not have a good framework yet to make that better.

Anyhow, a couple of answers regarding your questions.

> ISEL already has extensive heuristics for when and how to pull those constants back in to instructions. Why create new heuristics elsewhere that would require synchronization with this?


Because ConstantHoisting has a cost model that is supposed to model this kind of thing and this pass has a broader scope than ISel.

> First of all, I don’t really know how to make constants “more opaque”, as suggested above. How much more opaque can you get beyond using a bitcast for immdiates? I am new to LLVM, so forgive me if I’m just being naïve


Honestly, I do not know the details of ConstantHoisting. I thought there was a mechanism to mark bitcasted constant as opaque and that the basic block scope wouldn’t change any of that. Given your comment, this is not the case and this is unfortunate. Anyhow, please proceed with your ISel changes.

Thanks for your patience,
-Quentin


http://reviews.llvm.org/D11363