[PATCH] D67383: Add new optimization pass of Tree-Height-Reduction

[François Saint-Jacques via Phabricator via llvm-commits]

fsaintjacques added inline comments.


================
Comment at: lib/Transforms/Scalar/TreeHeightReduction.cpp:73
+
+// An option not to apply tree height reduction to floating-point instructions.
+static cl::opt<bool>
----------------
The `not` is misleading as this is disabled by default.


================
Comment at: lib/Transforms/Scalar/TreeHeightReduction.cpp:379
+  const int InstLatency =
+      TTI->getInstructionCost(Inst, TargetTransformInfo::TCK_Latency);
+  setLatency(getLatency() + InstLatency);
----------------
hfinkel wrote:
> This is the only call to TTI that I see, and I don't see it don't see it documented anywhere what objective function this transformation is effectively using. Can you please add some explanation of that? Also, I'd expect to see some check on the instruction throughput -- because it might not make sense to introduce more ILP than the target can use -- and I'd expect to see some register-pressure estimate.
@hfinkel I'd like to see this optimization in the next release. I don't have the time and knowledge to make the changes you request. Is it an acceptable solution if I disable this optimization in all levels, but make it accessible to front-end languages?

The cost estimation would be a followup task.


================
Comment at: test/Transforms/TreeHeightReduction/AArch64/fp16-mult.c:1
+; RUN: opt -O1 -enable-fp-thr -S < %s | FileCheck %s
+
----------------
xbolva00 wrote:
> This should be fp16-mult.ll, not fp16-mult.c
> 
> Please adjust tests to test only this new pass, not the -O1 pipeline.
I assume it's the same for all other files.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67383