[PATCH] D114895: [SelectionDagBuilder] improve CallBrInst BlockAddress constraint handling

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D114895#3177794 <https://reviews.llvm.org/D114895#3177794>, @efriedma wrote:

> In D114895#3170282 <https://reviews.llvm.org/D114895#3170282>, @nickdesaulniers wrote:
>
>> Unless you can tie a block address. Hopefully `"+r"(&&goto_label)` isn't a valid output constraint...
>>
>>   void x(void) {
>>       asm goto ("":"+r"(&&foo):::foo);
>>       //                ^ error: lvalue required in 'asm' statement
>>       foo:;
>>   }
>
> I think you meant to write something like this:
>
>   void *x(void) {
>       void *p = &&foo;
>       asm goto ("":"+r"(p):::foo);
>       foo:;
>       return p;
>   }
>
>
>
> -----
>
> Sorry, I think my original comment led you in the wrong direction.  Thinking about this a bit more, I think the solution is actually something like the following: treat any blockaddress passed to an "X" constraint the same way, regardless of whether there's a callbr involved, and regardless of the position in the input list.  I think that comes closest to matching the original design here.
>
> The "with an 'X' constraint" part avoids messing with users passing a blockaddress as a register input or something like that.
>
> Actually, not sure why clang is emitting "X" for asm goto destinations, instead of "i".  "i" is explicitly defined to be an immediate, and it's not clear what "X" is supposed to mean.  So maybe better to do something like the following:
>
> 1. Make clang emit "i" instead of "X" for asm goto destinations.  LLVM already handles blockaddress passed to "i" correctly.
> 2. Drop the special case for blockaddress in SelectionDAGBuilder::visitInlineAsm.
> 3. Optionally, add some extra handling for "X" in the backend, for backwards compatibility with IR generated by old clang.

1a. https://reviews.llvm.org/D115410
1b. https://reviews.llvm.org/D115311

2. https://reviews.llvm.org/D115409
3. <not done/is this necessary?>



In D114895#3181386 <https://reviews.llvm.org/D114895#3181386>, @efriedma wrote:

> In D114895#3181302 <https://reviews.llvm.org/D114895#3181302>, @nickdesaulniers wrote:
>
>> if you change the asm string to use `%l2` rather than `%l1`, it compiles (and emits the expected code).  I think this is another incompatibility that is likely to harm us in the future.  I'm not sure yet whether this is orthogonal to this issue. (I'm not sure whether this is a bug or a feature in GCC; it means that the %0,%1,%2 references in the asm string are ordered: outputs, non-tied-inputs, inputs-tied-to-outputs, then goto-labels.  Clang today emits them as outputs, non-tied-inputs, goto-labels, then inputs-tied-to-outputs.
>
> Ouch.  We probably need to fix that...



4. https://reviews.llvm.org/D115471

>> Finally, I had a thought last night that I've always felt that `callbr` had a minor design issue that never quite sat right with me. https://reviews.llvm.org/D74947 demonstrates how methods like `User::replaceUsesOfWith` can subtly break `callbr`. Since `callbr` duplicates it indirect destinations twice in its operand list, once for the arg list to the `call asm` and again just cause (ie. the labels used as indirect destinations show up twice in the actual IR statement), replacing operands is brittle because the duplicate operands need to stay in sync.  If the indirect list at the end used placeholders more akin to `std::placeholders`, then we would know precisely which argument are intended to be indirect destinations, and avoid brittle duplication that breaks when calling `User::replaceUsesOfWith`.  This would be extensive surgery to `callbr`, but I'd sleep better at night knowing that `callbr` was less brittle.
>
> The current design allows you to use blockaddress constants in code outside the asm goto to compute the destination of an asm goto, I think?  At least, the IR definition looks like it allows that; not sure how well it works in practice.
>
> If we're willing to abandon this, we could use an alternate design where we don't use blockaddress at all.  Instead, asm callbr has implicit arguments: we just allow the asm to refer to the destinations labels directly.  So for a callbr with no explicit arguments, "$0" refers to the first destination.  The primary benefit here is that we can remove a bunch of special cases for callbr control flow at the IR level.  (I expect that in/after SelectionDAG, we'd keep the current representation?  At least, we can consider that separately.)

So the idea I had (I should probably post this to llvm-dev, rather than re-use this likely to be abandoned code review) was to change:

  callbr void asm sideeffect "", "r,X"(i64 42, i8* blockaddress(@test5, %2))
            to label %1 [label %2]

to

  callbr void asm sideeffect "", "r,X"(i64 42, i8* blockaddress(@test5, %2))
            to label %1 [i64 1]

(Note: the only difference is in the indirect destination list at the end within the `[]`).

In the first case, the label `%2` shows up twice within the operand list of the `callbr`. Once near the end as an indirect destination, but again in the `blockaddress` argument list to the `asm`. Those two need to stay in sync, and D74947 <https://reviews.llvm.org/D74947> demonstrates how easily `User::replaceUsesOfWith` can force those repeated operands out of sync.

In the second case, I just use a literal `i64 1` which is an index into the argument list (`i64 0` would refer to `i64 42`; the 0th arg to the `asm`).  This is akin/reminiscent of `std::placeholders` that one might use with `std::bind`.  I don't think we ever reorder operands to an `asm` call, so we can use `User::replaceUsesOfWith` on the `blockaddress` operands/arguments all we want without the indices getting messed up.

This would also have the side effect of fixing the ambiguity described in this exact phab review. For an input like:

  ; see test5 from this phab review above
  callbr void asm sideeffect "# $0\0A\09# ${1:l}\0A\09", "X,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@test5, %2), i8* blockaddress(@test5, %2))
            to label %1 [i64 1]

It's no longer ambiguous which of the 2 matching arguments is the indirect destination; the latter is! So disambiguation and making callbr less brittle to `User::replaceUsesOfWith` are two benefits IMO of doing such a change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114895