In D142556#4103178, @dblaikie wrote:

Numbers look fairly good - wonder if you could get this run through the LLVM perf tracker to get some more stable/precise numbers?
& any chance you could use something like bloaty ( https://github.com/google/bloaty ) or by hand comparison on a per-section basis on the clang binary? Getting a sense of, specifically, how much bigger this makes .debug_info (but any incidental changes to other sections too) might be helpful

In D142556#4103178, @dblaikie wrote:

Numbers look fairly good - wonder if you could get this run through the LLVM perf tracker to get some more stable/precise numbers?
& any chance you could use something like bloaty ( https://github.com/google/bloaty ) or by hand comparison on a per-section basis on the clang binary? Getting a sense of, specifically, how much bigger this makes .debug_info (but any incidental changes to other sections too) might be helpful

LGTM.

Yeah, GDB had implemented it as GNU extension even before LLDB.
All in all, I don't see any obstacle to use the entry_value for this case. GCC also produces the same location entry for the variable b.

Hello. I am wondering, have you checked how both `GDB`` and `LLDB`` act with this example?

Looks reasonable to me.

Great!

Please add a test.

@nlopes Thanks again for reporting this. The problem is we do not check if the global variable representing the table was a constant. The patch needed looks as:

In rGdf868edee561eb973edd85ec9df41c67aa0bff6b#1124141, @nlopes wrote:

Alive2 finds a bug in this commit:

@ctz6.table = global 256 bytes, align 4

define i32 @ctz6(i64 %n) denormal-fp-math=ieee,ieee {
%entry:
  %add = sub i64 0, %n
  %and = and i64 %add, %n
  %mul = mul i64 %and, 571347909858961602
  %shr = lshr i64 %mul, 58
  %arrayidx = gep inbounds ptr @ctz6.table, 256 x i64 0, 4 x i64 %shr
  %0 = load i32, ptr %arrayidx, align 4
  ret i32 %0
}
=>
@ctz6.table = global 256 bytes, align 4

define i32 @ctz6(i64 %n) denormal-fp-math=ieee,ieee {
%entry:
  %0 = cttz i64 %n, 1
  %1 = icmp eq i64 %n, 0
  %2 = select i1 %1, i64 63, i64 %0
  %3 = trunc i64 %2 to i32
  ret i32 %3
}
Transformation doesn't verify! (unsound)
ERROR: Value mismatch

Example:
i64 %n = #xf600000000000040 (17726168133330272320, -720575940379279296)

Source:
i64 %add = #x09ffffffffffffc0 (720575940379279296)
i64 %and = #x0000000000000040 (64)
i64 %mul = #xfb757966938a3080 (18119522157263990912, -327221916445560704)
i64 %shr = #x000000000000003e (62)
ptr %arrayidx = pointer(non-local, block_id=0, offset=248)
i32 %0 = #x00000000 (0)

Target:
i64 %0 = #x0000000000000006 (6)
i1 %1 = #x0 (0)
i64 %2 = #x0000000000000006 (6)
i32 %3 = #x00000006 (6)
Source value: #x00000000 (0)
Target value: #x00000006 (6)

The bug seems to be that the code doesn't check that the global @ctz6.table is constant.

LGTM, thanks

In D113291#3777195, @spatel wrote:

In D113291#3777119, @djtodoro wrote:

@rsmith recommitted with f879939157. Thanks!

What was the bug, how was it fixed, and is there a new test to verify the fix? That should have been mentioned in the new commit message.

Thanks for doing this. Can you please check the impact of this on a benchmark (e.g., SPEC or https://llvm.org/docs/TestSuiteGuide.html) ?

@rsmith recommitted with f879939157. Thanks!

Thanks a lot. I will check ASAP.

Very interesting. This looks good to me. Thanks.

lgtm, thanks

• addressing comments

Thanks for your comments.

• addressing comments

• clang-format
• clean up tests

• removed more duplicated tests
• added a llvm/test/Transforms/PhaseOrdering test
• refactor the code a bit

Thanks for the comments.

• adding negative tests
• rename the tests
• clean the target triple leftovers from tests

In D113291#3728796, @spatel wrote:

Why are some test files still specifying a triple in the RUN line?

Leftovers. Thanks.

• remove a duplicated test

• support opaque pointers
• remove leftovers (since this was aarch64 only)
• move the tests in a non-target dir

I promise I will find some time to update this - it is coming next week.

I am wondering, what happens if we use GlobalIsel as instruction selector?

@xry111 Thanks for this! Since the implementation of TLS in glibc may differ in various versions (but should be very constant after ~2.19), is there anything here that depends on the glibc version?

lgtm

lgtm

In D113291#3569456, @gsocshubham wrote:

In D113291#3569225, @djtodoro wrote:

In D113291#3569114, @gsocshubham wrote:

In D113291#3566581, @djtodoro wrote:

In D113291#3542941, @efriedma wrote:

Concretely, my preferred solution looks something like:

1. Perform the transform unconditionally in AggressiveInstCombine (so this patch without the preferCTTZLowering() bits).

The latest update implements this.

2. Teach TargetLowering::expandCTTZ to emit a table lookup.

@djtodoro - Will you be sending patch for (2) "Teach TargetLowering::expandCTTZ to emit a table lookup."?

Unfortunately, I don’t have time to do it right now. If you are interested, please go ahead with the implementation.

@djtodoro - Sure. I am interested to do it. Can you elaborate (2) a bit in detail?

In D113291#3569114, @gsocshubham wrote:

In D113291#3566581, @djtodoro wrote:

In D113291#3542941, @efriedma wrote:

Concretely, my preferred solution looks something like:

1. Perform the transform unconditionally in AggressiveInstCombine (so this patch without the preferCTTZLowering() bits).

The latest update implements this.

2. Teach TargetLowering::expandCTTZ to emit a table lookup.

@djtodoro - Will you be sending patch for (2) "Teach TargetLowering::expandCTTZ to emit a table lookup."?

In D113291#3542941, @efriedma wrote:

Concretely, my preferred solution looks something like:

1. Perform the transform unconditionally in AggressiveInstCombine (so this patch without the preferCTTZLowering() bits).

The latest update implements this.

• drop target dependent hooks

Thanks for this.

LGTM from my side.

This is interesting! Can you please make a simple/dedicated LLVM IR case for this only, so we can go over the example to get the whole picture?

Thanks a lot for the comments! Can someone please sum the things up that need to be done for this?

In D113291#3539965, @efriedma wrote:

as for the second I'm not sure when it would be profitable to transform back and emit the table

You really just have to weigh it against the current default expansion on targets where ctlz/cttz aren't legal, which is popcount(v & -v). It should be a straightforward comparison, generally. If you have popcount, use it. If multiply is legal, use a table lookup. Otherwise... maybe stick with the popcount expansion? Probably any approach is expensive at that point.

Compare the generated code for arm-eabi.

In D113291#3539840, @dmgreen wrote:

Hello - We were having a discussion about a very similar patch in D125755. I think the outcome for this patch is that either:

• We need to do this later (maybe in CodeGenPrepare).

sounds reasonable to me

But I think that x86 support doesn’t block this.

OK, great! it will be on my TODO list!

Thanks!

In D113291#3493276, @xbolva00 wrote:

How hard is to add x86 support?

@xbolva00 ping :)

Thanks!

Thanks for this! Please add test.

Please update https://llvm.org/docs/HowToUpdateDebugInfo.html#test-original-debug-info-preservation-in-optimizations with this. Other than that, looks good to me.

Hi @impiaaa, thanks for working on this!
Since this is an initial support, I was wondering is there a clear set of TODOs that need to be done?