[PATCH] D89390: [TargetLowering] Update optimization to check for boolean content

Sun Oct 18 10:38:55 PDT 2020

ErikHogeman added a comment.

> Let's add the nvptx test with current codegen as a preliminary step. That way, we'll at least have better coverage for today's behavior.
> Then, it's a question of how to expose the buggy behavior. I'm not familiar with nvptx; can we make a larger test that would still show a diff? Is your motivating example/target public, or can we adapt the larger example to work with nvptx?
> There's another way to improve this whole block and probably demonstrate the bug on multiple in-tree targets if you're up for it: fix the TODO to allow vector types.
> If none of the above, then we can continue as originally proposed without a regression test.

The original IR that triggered this issue is very large, and unfortunately for a target not checked in upstream, so cannot be used as-is for a test. I have tried cutting it down a bit and keep the relevant parts but I have not been able to get the bug to trigger, either the SETCC is rewritten in some way early or it stays as an i1 type and doesn't trigger the bug. I am also not too familiar with nvptx so not sure what conditions would trigger the SETCC to be rewritten to a larger type. I spent a bit more time trying to cut down the original problematic IR but I'm struggling to reproduce it.
If there was a way to write unit tests directly for the selection DAG (so you could specify exactly which DAG node patterns to test) it would have been easy to write a test I think.

Sorry just to clarify that I understood you correctly, would you prefer that I add a test with the nvptx code using a SETCC with i1 type, or would you rather I change the patch to not abort on i1 types since that resulted in more instructions in the example you posted?
Uploading this example as a test works for me, but I'm slightly concerned that it seems to generate more instructions now in a case where it should not have to do that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89390