[PATCH] D127831: BasicBlockUtils: Add a new way for CreateControlFlowHub()

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon marked 2 inline comments as done.
bcahoon added a comment.

In D127831#3891852 <https://reviews.llvm.org/D127831#3891852>, @foad wrote:

> In D127831#3890092 <https://reviews.llvm.org/D127831#3890092>, @bcahoon wrote:
>
>> In D127831#3887582 <https://reviews.llvm.org/D127831#3887582>, @foad wrote:
>>
>>> Why do we still need the "old way"? Does the "new way" generate worse code in some cases?
>>
>> Good question. The new way does require an additional compare instruction to test where control continues after the loop. One for each output edge. But, yes, there is a tradeoff here. I think it's worth assessing if the "new way" can replace the "old way" at some point. My thinking here is to be conservative and keep the "old way" for now as I'm a little hesitant to replace it without more performance data.
>
> Right, I understand that using an i32 instead of an i1 means that you need extra icmp instructions in the IR, but does it actually cause more instructions in the final generated code (for AMDGPU)? If the i1 was stored in a general purpose register then you would need a cmp //anyway// to use it to control a conditional branch.

Only a handful of code gen LIT tests fail if I enable the new way, with an integer value for all cases. I was surprised it was so few. The code differences in the tests are not significant. When boolean values are used, the compiler generates and instructions, and when the integer value is used the compiler generate cmp instructions instead. Some other minor differences in one of the tests, e.g., moving a constant to an sgpr. I can try to find and run some other tests and check the generated code.


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

https://reviews.llvm.org/D127831