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

[Zia Ansari via llvm-commits]

zansari added a comment.

Hi Quentin,

Thanks for the additional thoughts and suggestions.

However, I’m still not comfortable with having ConstantHoisting doing the transformation. I’ll try to address your concerns, and break things down a little more. Hopefully I’ll address most of your questions and concerns.

- I’ll start by giving my opinion on what’s “natural” with respect to subsuming constants.
  - ISEL will always pull out all constants within BBs.
  - 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?
  - It seems extremely natural to simply modify those existing heuristics just slightly to catch EXACTLY the cases that we want. These can be easily tuned to catch more/fewer case as we desire.
    - Doing it in ConstantHoisting is clumsy in that it will blindly pull out constants.
    - Adding heuristics to ConstantHoisting to be smarter about what it pulls out needs careful and fragile synchronization with ISEL for various targets, which could get ugly.

- I DO agree that it would be nice to do this at a level where more targets could benefit, but my concern is with doing something that will, instead, hurt all targets.
  - 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 
    - ConstantHoisting uses bitcasts AND pulls said bitcasts out into dominating blocks, and calls them “opaque”.
      - These opaque constants, when appearing in their own BB will be lowered as constant instantiations by ISEL.
      - These opaque constants, when referenced from outside BBs, will references the instantiated constants.. however..
      - These opaque constants, when referenced in a block in which they’re defined, will get pulled into all references by ISEL (per Michael’s argument that they are not “opaque enough”).
    - Given that opaque constants get pulled into local BB users, we would require similar (to my patch) changes in ISEL to prevent this from happening.
    - I’d really prefer not to hoist the constants out of their BBs, as I’d like to avoid cross-BB live ranges. I’d like to tackle global constant hoisting in a separate effort, which will/should be handled in ConstantHoisting.cpp.
  - Let’s assume we can make these local BB constant “more opaque” to the point that ISEL can’t pull them into instructions anymore. This will have a few negative effects:
    - Without lots of changes, ISEL can’t pick and choose which constants might be OK for its target for specific instructions. It’s left to the mercy of everything ConstantHoisting has pulled out. That is, ALL targets will simply punt on pulling any of those constants back in. I think it needs to be a target-specific decision to pick instructions/constants to subsume. Moving all of the target-specific decision making into ConstantHoisting doesn’t seem practical.
    - Without lots of changes, DAG and ISEL will stop seeing constants and fail to do any optimizations that involve immediates. I’ll admit I haven’t spent tons of time trying to come up with concrete examples, but DAG and ISEL have hundreds of references and queries for Constants in .td and .cpp files. I’m certain a lot of these are optimizations. With “more opaque” constants, these will all fail if they have been pull out by ConstantHoisting. Again, I think it’s unreasonable to implement changes in ConstantHoisting to work together with ISEL.

- On to the phase ordering..
  - Yes, I agree that ConstantHoisting runs late, but that seems to be a big restriction. Anything that runs on the IR after ConstantHoisting today, or tomorrow, will suffer from not being able to process immediates, since they are now all pulled out and “more opaque”. Given the frequency of 8/16/32bit immediates, this will have a large impact on the IR and everyone walking it.
  - Even if we were guaranteed that ConstantHoisting was always the very last phase to run before DAG/ISEL, we’ll still be missing opportunities. DAG itself introduces lots of constants in call lowering, address lowering, and perhaps a few other places. I actually messed with my patch a little to make it more aggressive to demonstrate this. Take this simple example:

  int x, y, p;
  double d;
  int foo()
  {
      p = 16;
      x = 4;
      y = 8;
      bar(4, 8, d, 4);
  }

By doing the merging in ISEL, I get this (note the stack offsets):

  subl      $28, %esp
  movl      $16, %eax
  movl      %eax, p
  movl      $4, %ecx
  movl      %ecx, x
  movl      $8, %edx
  movl      %edx, y
  movsd     d, %xmm0
  movl      %ecx, (%esp,%eax)
  movsd     %xmm0, (%esp,%edx)
  movl      %edx, (%esp,%ecx)
  movl      %ecx, (%esp)
  calll     bar



- Which I thought was kind of neat, since I’ve never seen this sort of optimization in a compiler :) Those stores to the stack are all smaller than their immediate offset counterparts. You can do similar optimizations with address expansion, also.
- You simply can’t do this with ConstantHoisting.

In summary, I’d like to emphasize what I think is the most important point : “ISEL already has extensive heuristics for when and how to pull constants back in to instructions. Why create new ones elsewhere that would require synchronization with this?”

However, since we have no other place that naturally pulls out and subsumes global (cross BB) constants, I’m in 100% agreement that ConstantHoisting should tackle those.

Thanks,
Zia.


http://reviews.llvm.org/D11363