Skip to content

SimplifyCFG switch removal should add no-wrap flags when possible #59671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
scottmcm opened this issue Dec 23, 2022 · 5 comments
Closed

SimplifyCFG switch removal should add no-wrap flags when possible #59671

scottmcm opened this issue Dec 23, 2022 · 5 comments

Comments

@scottmcm
Copy link

scottmcm commented Dec 23, 2022

Today, LLVM optimizes

define noundef i8 @src(i8 noundef %x) unnamed_addr {
start:
  switch i8 %x, label %bb3 [
    i8 -1, label %bb1
    i8 0, label %bb5
    i8 1, label %bb2
  ]

bb3:                                              ; preds = %start
  unreachable

bb5:                                              ; preds = %start
  br label %bb1

bb2:                                              ; preds = %start
  br label %bb1

bb1:                                              ; preds = %start, %bb5, %bb2
  %.0 = phi i8 [ -1, %bb2 ], [ 0, %bb5 ], [ 1, %start ]
  ret i8 %.0
}

down to

start:
  %switch.offset = sub i8 0, %x
  ret i8 %switch.offset

That's a huge improvement, but it could be slightly better as a sub nsw instead: https://alive2.llvm.org/ce/z/GZs_dj

If I'm understanding things right, SimplifyCFG turns the switch into

define noundef i8 @tgt(i8 noundef %x) unnamed_addr {
start:
  %switch.tableidx = sub i8 %x, -1
  %switch.idx.mult = mul i8 %switch.tableidx, -1
  %switch.offset = add i8 %switch.idx.mult, 1
  ret i8 %switch.offset
}

but in this case it could (https://alive2.llvm.org/ce/z/XzPeQv) have those be sub nsw, mul nsw, and add nsw, to better preserve the knowledge of the limited input range -- or maybe special case switches that map 0 to 0 to just emit mul nsw (or nuw) when legal without the extra sub/add complication.

Then something else (instcombine?) could take advantage of those nsw flags to optimize down to just the sub nsw i8, 0, %x: https://alive2.llvm.org/ce/z/tESh4R. But it looks like they got lost right now (https://llvm.godbolt.org/z/3dMW8j1sr), so maybe there'd be more work there too. (If SimplifyCFG left off the sub/add, though, and just had the mul nsw, then InstCombine looks like it would already preserve the nsw to the sub https://llvm.godbolt.org/z/9der4sraG.)


Original rust exploration that led me here: https://rust.godbolt.org/z/EW1aba8Mr

@khei4
Copy link
Contributor

khei4 commented Mar 7, 2023

nit: Currently arithmetically negated branch optimization has not been implemented even on the main. But logical negation is implemented.
https://llvm.godbolt.org/z/8ozvqj1cf

But InstCombine part seems reasonable.

Edit: Sorry I misunderstand the related pass and just have seen the partial IR result(missing data layout) https://llvm.godbolt.org/z/7saozhM5K.

Original rust exploration that led me here: https://rust.godbolt.org/z/EW1aba8Mr

@khei4
Copy link
Contributor

khei4 commented Mar 26, 2023

reduced case for opt -O3, (SimplifyCFG switch-to-lookup depends on native integer widths)
https://llvm.godbolt.org/z/1Ynvjqvnn
for SimplifyCFG, switch-to-lookup enabled
https://llvm.godbolt.org/z/nraenEz7j

@nikic
Copy link
Contributor

nikic commented May 19, 2023

Does the missing nsw flag actually matter for some following optimization?

@nikic
Copy link
Contributor

nikic commented Jun 26, 2023

The last part of this has landed in https://reviews.llvm.org/D150943, so closing this issue. We how produce sub nsw i8 0, %x for this input.

@nikic nikic closed this as completed Jun 26, 2023
@scottmcm
Copy link
Author

Thanks nikic!

I forget specifically what I was looking at back when I opened this, so I'm not certain what, if anything, needed it. My guess would be something like a c.reverse() < 0 after a bunch of inlining, and thus knowing that it's nsw is important to turn that into c > 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants