[PATCH] D139000: [AMDGPU] Clear bodies of function with incompatible features

Thu Dec 1 07:32:53 PST 2022

Joe_Nash added a comment.

In D139000#3962770 <https://reviews.llvm.org/D139000#3962770>, @Pierre-vh wrote:

> In D139000#3960912 <https://reviews.llvm.org/D139000#3960912>, @Joe_Nash wrote:
>
>> Overall this looks pretty good. As you say, the feature checking logic is quite limited, but that's not a problem. 
>> I think after this patch lands https://reviews.llvm.org/D123693 can be reverted. Can you try to revert that with this patch and check if device libs can be built correctly at -O0?
>
> I did a quick test where I passed all .bc files from the device libs to Clang for fiji (gfx8) and it still doesn't build even with this patch. The pass kicks in a few times but there's some issues with "dot" instructions.
> Not sure how to address those - should it be done in this pass? For instance device libs has a few places here it uses `(target("dot8-insts")` which allows selection to work (because that only checks the feature) but then it fails because there is no "real" instruction for GFX8 dot8, only GFX11 (it uses the generation).
>
> Do I just go "whack a mole" and try to build, add more checks, try to build again, etc?
> I'm worried about complexity exploding if the checks need to be more intricate. e.g. I see that dot instructions have been introduced in the middle of the GFX9 generation (GFX908?) so I'd already need to change the pass completely to check for GFX908

Thanks for looking deeper into this. I believe it is the right functional test for this patch if it can correctly build device libs with D123693 <https://reviews.llvm.org/D123693> reverted. So I would recommend continuing the implementation till that is true.

I would recommend one level of abstraction on the attribute checks so it is easier to extend. So something like this pseudocode.

  foreach ( attribute in function)
      if (isIllegal(attribute, function.subtarget))
          remove = true;

isIllegal can be similar to your current FeatureAndMinGen, since you mentioned before there is no existing api for determining if an attribute is legal on a subtarget (though I'm surprised by that and think if you build it the users will come).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139000