[PATCH] D129288: [IR] Don't use blockaddresses as callbr arguments

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

> Why the poison placeholders? This makes it easy to satisfy the existing inlineasm infrastructure. It would be cleaner to omit the placeholder arguments and just say that trailing inlinasm constraints refer to the indirect target labels. InlineAsm itself doesn't have knowledge about those though, so it would not be possible to verify InlineAsm constraints in isolation.

I'm really uncomfortable with this hack. If we're going to introduce a new kind of input which doesn't come from the arglist, I think it'd be better to introduce a new constraint modifier symbol which can be used with input constraints to indicate that...then, the parsed InlineAsm will have the expected count of those separately, and the the verifier can verify the count matches at the appropriate time (during call/callbr verification), and we can propagate this into SDag, etc...

E.g., we could choose the symbol '!' for that, so you might have e.g.:
`callbr void asm sideeffect "", "!i,r,!i,~{dirflag},~{fpsr},~{flags}"(i32 4) to label %1 [label %2, label %3]`

In D129288#3635747 <https://reviews.llvm.org/D129288#3635747>, @nikic wrote:

> In D129288#3635732 <https://reviews.llvm.org/D129288#3635732>, @jyknight wrote:
>
>> Can't we remove blocks from the callbr target list in some circumstances? E.g. if we merge two blocks maybe?
>
> I believe we currently don't allow doing that, see e.g. the check in https://github.com/llvm/llvm-project/blob/26f369393d4e60c78d872c4cafcbf3fd929b9004/llvm/lib/Transforms/Utils/Local.cpp#L1092. This is because we currently don't allow callbr to have the same successor multiple times (though I think we //should// allow that, to allow transforms like these.)

Huh. I would've expected us to previously have coalesced duplicates, not prohibit rewrites that would create duplicates. Anyways, NOW we should definitely allow duplicates in the list -- it's now actually an ordered list, not a set as it theoretically was before. If you've gone through the code to ensure that nothing ever modifies the order of entries, or adds/removes entries from the list, we'll probably be okay after that change.


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

https://reviews.llvm.org/D129288