[PATCH] D91734: [FastISel] Flush local value map on every instruction

Thu Dec 3 10:33:03 PST 2020

dblaikie added a comment.

In D91734#2431247 <https://reviews.llvm.org/D91734#2431247>, @probinson wrote:

>> Sometihng like this seems plausible to me:
>
> Yes, I was playing with essentially that exact patch last night.  It has no effect on the final assembly on its own, but combined with my patch it does.

It might have effects on assembly in other test cases, though. Could be worth running it through a self-host or something to see what other changes it causes and whether they're desirable.

>> (a more general fix (that would cover cases where the instruction really has no location) might be to propagate locations from the first instruction in a basic block with a location back up to the start of the basic block - I forget if we've considered/tried that before)
>
> We have, but that without flushing the map on every instruction, so it caught materialization instructions that didn't actually belong to that IR instruction.  The tactic would likely be more reasonable in conjunction with my patch.

(oh, when I was saying that I didn't really think - the materialization in this case wasn't necessarily on a BB boundary - so I guess my suggestion amounts to possibly never using line 0 (unless it's the only location in a whole BB), instead back or forward propagating surrounding locations over any line 0 - and that doesn't sound right when I put it that way)

But yeah, maybe some amount of it could be done around the flushing thing.

(FWIW, about this patch in general, I do worry a bit about this being a debug-info motivated optimization decision (even if that decision is applied uniformly/not just when debug info is enabled) - you mention this might have positive performance features due to smaller register live ranges, but also possibly negative ones rematerializing the same constant (" however, only about 5% of those values
were reused in an experimental self-build of clang.") - do you have performance measurements/benchmarks related to this change? I guess it didn't show up in the perf bot profiles upstream at least)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734