[GISel] Explicitly disable BF16 tablegen patterns. #124113
Conversation
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-backend-aarch64 Author: David Green (davemgreen) ChangesWe currently have an issue where bf16 patters can be used to match fp16 types, as GISel does not know about the difference between the two types. This patch explicitly disables them to make sure that they are never used. Full diff: https://github.com/llvm/llvm-project/pull/124113.diff 2 Files Affected:
diff --git a/llvm/test/CodeGen/AArch64/fptrunc.ll b/llvm/test/CodeGen/AArch64/fptrunc.ll
index 2187717c4148ae..b4c38e9f2df3b2 100644
--- a/llvm/test/CodeGen/AArch64/fptrunc.ll
+++ b/llvm/test/CodeGen/AArch64/fptrunc.ll
@@ -1,6 +1,7 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 3
; RUN: llc -mtriple=aarch64 -global-isel=0 -verify-machineinstrs %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-SD
; RUN: llc -mtriple=aarch64 -global-isel=1 -verify-machineinstrs %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-GI
+; RUN: llc -mtriple=aarch64 -global-isel=1 -mattr=+fullfp16,+bf16 -verify-machineinstrs %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-GI
define float @fptrunc_f64_f32(double %a) {
; CHECK-LABEL: fptrunc_f64_f32:
diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp
index 5466d315c05a49..2969dd9156ccbf 100644
--- a/llvm/utils/TableGen/GlobalISelEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp
@@ -2346,6 +2346,20 @@ void GlobalISelEmitter::emitRunCustomAction(raw_ostream &OS) {
<< "}\n";
}
+bool hasBFloatType(const TreePatternNode &Node) {
+ for (unsigned I = 0, E = Node.getNumTypes(); I < E; I++) {
+ auto Ty = Node.getType(I);
+ for (auto T : Ty)
+ if (T.second == MVT::bf16 ||
+ (T.second.isVector() && T.second.getScalarType() == MVT::bf16))
+ return true;
+ }
+ for (const TreePatternNode &C : Node.children())
+ if (hasBFloatType(C))
+ return true;
+ return false;
+}
+
void GlobalISelEmitter::run(raw_ostream &OS) {
if (!UseCoverageFile.empty()) {
RuleCoverage = CodeGenCoverage();
@@ -2382,6 +2396,13 @@ void GlobalISelEmitter::run(raw_ostream &OS) {
if (Pat.getGISelShouldIgnore())
continue; // skip without warning
+
+ // Skip any patterns containing BF16 types, as GISel cannot currently tell
+ // the difference between fp16 and bf16. FIXME: This can be removed once
+ // BF16 is supported properly.
+ if (hasBFloatType(Pat.getSrcPattern()))
+ continue;
+
auto MatcherOrErr = runOnPattern(Pat);
// The pattern analysis can fail, indicating an unsupported pattern.
|
There was a problem hiding this comment.
This fixes a codegen/runtime error that we are seeing, as reported in #120363 (comment)
I am not a GISel guy, but I have applied and verified that this patch solves the problems that we are seeing, and the change looks very reasonable, so am confident to LGTM this.
Thanks for the quick fix!
|
Do we have a tracking issue for bf16? Should mention to revert this there |
|
I am relatively new to GISel intersection FP16. In the longer term, will we have a clean solution when GISel represents types natively? As Matt said above, an issue would be good to have and link to #115133? |
I couldn't see one for BF16 specifically, but I'll add a note to #115133 and FYI @tgymnich if you run into this. Hopefully it is fairly obvious that none of the patterns are importing or types work when we can use bf16 correctly. Let me know if you know of a better place, quite a lot has changed now.. Sorry - This is running into other issues, where the opposite happens for certain intrinsics and instructions with fp16 types are using bf16 patterns. As in https://godbolt.org/z/1KcvszrY7. I have applied the fairly heavy hammer of forcing all IR with bf16 types to fall back to SDAG. This shouldn't be a problem for data-processing only instructions (load, stores, call params, shuffles, inserts, extract, select, etc there are a lot of them), but they are currently included to make sure everything is covered properly. |
davemgreen
force-pushed
the
gh-gi-disablebf16pats
branch
from
7e79a06 to
a8414e3
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
davemgreen
force-pushed
the
gh-gi-disablebf16pats
branch
from
a8414e3 to
e33932c
Compare
We currently have an issue where bf16 patters can be used to match fp16 types, as GISel does not know about the difference between the two types. This patch explicitly disables them to make sure that they are never used. The opposite can also happen too, where fp16 patterns are used for operators that should be bf16. So any operations with bf16 types now cause a fallback to SDAG. For the moment this includes data-processing only instructions (loads, stores, shufles, etc). The pass setup for GISel has been slightly adjusted to make sure that a verify pass does not get added between AMD-SDAG and SIFixSGPRCopiesPass, which otherwise can cause verifier issues when falling back.
davemgreen
force-pushed
the
gh-gi-disablebf16pats
branch
from
e33932c to
7af3b30
Compare
|
I've tried to clean this up to avoid typesize-only operations that are already tested and include just the instructions that cause problems. Hopefully it has not missed any, and calls are still excluded entirely. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/14690 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/12788 Here is the relevant piece of the build log for the reference |
|
Hi @davemgreen With this patch fails when llc is compiled with EXPENSIVE_CHECKS on: Can also be seen if you use a normal build and just add "-verify-machineinstrs" to the command on line 3: |
|
Thanks for the heads up - it looks like after we fall back from global-isel to SDAG, the verifier gets called, which calls getReservedRegs which uses SIMachineFunctionInfo::usesAGPRs which caches the result of UsesAGPRs. Because we have just fallen-back the function is empty and it incorrectly gets cached to false. This then causes a regallocation failure. One way around it might be to make sure we do not call getReservedRegs on an empty function in the verifier, but it may be better to reset that cached value somewhere, I'm just not sure where. |
|
New, hopefully better idea in #124799 |
We currently have an issue where bf16 patters can be used to match fp16 types, as GISel does not know about the difference between the two. This patch explicitly disables them to make sure that they are never used.
The opposite can also happen too, where fp16 patterns are used for operators that should be bf16. So this also changes any operations with bf16 types to now cause a fallback to SDAG.
The pass setup for GISel has been slightly adjusted to make sure that a verify pass does not get added between AMD-SDAG and SIFixSGPRCopiesPass, which otherwise can cause verifier issues when falling back.