[LLVMbugs] [Bug 23389] New: Intrinsics llvm.read_register, llvm.write_register and ISel node types ISD::READ_REGISTER, ISD::WRITE_REGISTER are broken.

[bugzilla-daemon at llvm.org]

https://llvm.org/bugs/show_bug.cgi?id=23389

            Bug ID: 23389
           Summary: Intrinsics llvm.read_register, llvm.write_register and
                    ISel node types ISD::READ_REGISTER,
                    ISD::WRITE_REGISTER are broken.
           Product: new-bugs
           Version: trunk
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P
         Component: new bugs
          Assignee: unassignedbugs at nondot.org
          Reporter: Nicholas.Paul.Johnson at DEShawResearch.com
                CC: llvmbugs at cs.uiuc.edu
    Classification: Unclassified

Created attachment 14275
  --> https://llvm.org/bugs/attachment.cgi?id=14275&action=edit
Testcase of register-asm syntax

(0) Background

I am developing a backend for a custom architecture.  Our ABI reserves a few
registers which we reserve for the application.  Those registers are accessed
from the C++-language by use of the assembly register syntax, like so:

  register int r0 asm("r0");

Clang lowers these accesses in LLVM IR using the llvm.read_register and
llvm.write_register intrinsics.



There is a minor difficulty while reporting this bug, since you do not have the
code for my backend.  Nonetheless, the bug is in the llvm mainline and will
manifest in any backend that exposes registers in this manner.

I'll try to explain this bug with respect to x86.  To do that, we need a minor
change to X86 so that it exposes register r15 at the C language level:

diff --git a/lib/Target/X86/X86ISelLowering.cpp
b/lib/Target/X86/X86ISelLowering.cpp
index 98a19ee..91f2b8d 100644
--- a/lib/Target/X86/X86ISelLowering.cpp
+++ b/lib/Target/X86/X86ISelLowering.cpp
@@ -15598,6 +15598,7 @@ unsigned X86TargetLowering::getRegisterByName(const
char* RegName,
   unsigned Reg = StringSwitch<unsigned>(RegName)
                        .Case("esp", X86::ESP)
                        .Case("rsp", X86::RSP)
+                       .Case("r15", X86::R15)
                        .Default(0);
   if (Reg)
     return Reg;



(1) Symptoms

Compilation gives different results under -O0 and -O3.  To reproduce:

Compile
  clang++ -O0 -c -emit-llvm -S bug.cpp -o bug.o0.ll
  clang++ -O3 -c -emit-llvm -S bug.cpp -o bug.o3.ll


Specifically, bug.o0.ll will re-load the latest value of the register (via
llvm.read_register) before each printf.  From bug.o0.ll:

   ...
   call void @_Z4zerov()
   %0 = call i64 @llvm.read_register.i64(metadata !0)
   store i64 %0, i64* %observation1, align 8
   %1 = load i64, i64* %observation1, align 8
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([12 x i8],
[12 x i8]* @.str, i32 0, i32 0), i64 %1)
   call void @_Z3onev()
   %2 = call i64 @llvm.read_register.i64(metadata !0)
   store i64 %2, i64* %observation2, align 8
   %3 = load i64, i64* %observation2, align 8
   %call1 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([13 x i8],
[13 x i8]* @.str1, i32 0, i32 0), i64 %3)
   ...

In contrast, bug.o3.ll will only read the register once, providing the same
integer argument to each printf.

 ; Function Attrs: nounwind uwtable
 define i32 @main(i32 %argc, i8** nocapture readnone %argv) #0 {
 entry:
   tail call void @llvm.write_register.i64(metadata !0, i64 0) #1
   %0 = tail call i64 @llvm.read_register.i64(metadata !0)
   %call = tail call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([12 x
i8], [12 x i8]* @.str, i64 0, i64 0), i64 %0)
   %add.i = add i64 %0, 1
   tail call void @llvm.write_register.i64(metadata !0, i64 %add.i) #1
   %call1 = tail call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([13 x
i8], [13 x i8]* @.str1, i64 0, i64 0), i64 %0)
   ret i32 0
 }



Also note: if you try to compile directly to a binary, clang / llc will crash
with "Invalid child # of SDNode".  I'll discuss that symptom below also.



(2) Analysis

There are several problems:

(2a) The intrinsic llvm.read_register is marked with IntrNoMem.  Consequently,
optimizations treat it as independent of llvm.write_register.  They incorrectly
reorder llvm.read_register w.r.t. llvm.write_register.
==> Instead, llvm.read_register should NOT have the IntrNoMem attribute, just
like llvm.write_register.

(2b) When SelectionDAGBuilder lowers llvm.write_register to
ISD::WRITE_REGISTER, it uses some nonsense value for the SDNode's chain
argument.  This is part of the "Invalid child" problem mentioned above.
==> Instead, the Chain operand should be from DAG.getRoot().

(2c) SelectionDAGISelLowering clobbers the value of the chain operand when it
translates ISD::WRITE_REGISTER into ISD::CopyToReg.
==> In should preserve Op->getOperand(0) rather than replacing it with
CurDAG->getEntryNode().



(3) Patch

I have a patch, but it seems can only submit one attachment at this time.  I'll
post a follow-up with the patch.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20150501/2e7e34e0/attachment.html>