[llgo] irgen: fix canAvoid*

[axw]

Summary

canAvoidElementLoad and canAvoidLoad were incorrectly
eliding loads when an index expression is used as an
another array index expression. This led to a panic.

See comments on https://github.com/go-llvm/llgo/issues/175

[pcc]

irgen/ssa.go

702 ↗

(On Diff #17322)

Can you leave the // ok comment here? This would make it a little clearer that we intend to accept the referrer rather than falling through.

704 ↗

(On Diff #17322)

(ignore this, phabricator is refusing to delete this comment)

718 ↗

(On Diff #17322)

If we make this hard-coded constant something like 2*sizeof(int), we can probably be guaranteed that we will never need to check for indexes below.

733 ↗

(On Diff #17322)

Likewise here.

735 ↗

(On Diff #17322)

Note that (as far as I can tell) this comparison would always fail because instr at this point cannot be an appropriate size to be used as an index. We might want to panic in the body of this if statement to make it clear what our assumptions are, or just revert this part.

[axw]

irgen/ssa.go

702 ↗

(On Diff #17322)

ok

718 ↗

(On Diff #17322)

Good point, array indices are always converted to int. Done.

733 ↗

(On Diff #17322)

Done.

735 ↗

(On Diff #17322)

I've changed it to panic, and added a comment in case someone makes the same mistake I did in the future.

[pcc]

LGTM

Do you have commit access?

[axw]

In D6676#102307, @pcc wrote:

LGTM

Do you have commit access?

Thanks. No, I don't.

[pcc]

Please do ask for it (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). In the meantime, I've committed this for you.

llgo/trunk/irgen/ssa.go

case *ssa.Global:
     llglobal = llvm.ConstBitCast(llglobal, fr.llvmtypes.ToLLVM(v.Type()))
     fr.globals[v] = llglobal
     return newValue(llglobal, v.Type())
   }
   if value, ok := fr.env[v]; ok {
     return value
   }
 
-  panic("Instruction not visited yet")
+  panic(fmt.Errorf("Instruction %q not visited yet", v.Name()))
 }
 
 func (fr *frame) llvmvalue(v ssa.Value) llvm.Value {
   if gv := fr.value(v); gv != nil {
     return gv.value
   } else {
     return llvm.Value{nil}
   }

 
 func (fr *frame) nilCheck(v ssa.Value, llptr llvm.Value) {
   if !fr.isNonNull(v) {
     ptrnull := fr.builder.CreateIsNull(llptr, "")
     fr.condBrRuntimeError(ptrnull, gccgoRuntimeErrorNIL_DEREFERENCE)
   }
 }
 
-func (fr *frame) canAvoidElementLoad(refs []ssa.Instruction) bool {
-  for _, ref := range refs {
-    switch ref.(type) {
-    case *ssa.Field, *ssa.Index:
+func (fr *frame) canAvoidElementLoad(ptr ssa.Value) bool {
+  for _, ref := range *ptr.Referrers() {
+    switch ref := ref.(type) {
+    case *ssa.Field:
+    case *ssa.Index:
+      if ref.X != ptr {
+        return false
+      }
       // ok
     default:
       return false
     }
   }
 
   return true
 }
 
 // If this value is sufficiently large, look through referrers to see if we can
 // avoid a load.
 func (fr *frame) canAvoidLoad(instr *ssa.UnOp, op llvm.Value) bool {
-  if fr.types.Sizeof(instr.Type()) < 16 {
+  if fr.types.Sizeof(instr.Type()) < 2*fr.types.Sizeof(types.Typ[types.Int]) {
     // Don't bother with small values.
     return false
   }
 
   // Keep track of whether our pointer may escape. We conservatively assume
   // that MakeInterfaces will escape.
   esc := false
 
   // We only know how to avoid loads if they are used to create an interface
   // or read an element of the structure. If we see any other referrer, abort.
   for _, ref := range *instr.Referrers() {
-    switch ref.(type) {
+    switch ref := ref.(type) {
     case *ssa.MakeInterface:
       esc = true
-    case *ssa.Field, *ssa.Index:
+    case *ssa.Field:
+    case *ssa.Index:
+      if ref.X != instr {
+        // This should never happen, as indices are always of type int
+        // and we don't bother with values smaller than 2*sizeof(int).
+        panic("impossible")
+      }
       // ok
     default:
       return false
     }
   }
 
   var opcopy llvm.Value
   if esc {
case *ssa.Extract:
     }
     elemtyp := instr.Type()
     fr.env[instr] = newValue(elem, elemtyp)
 
   case *ssa.Field:
     fieldtyp := instr.Type()
     if p, ok := fr.ptr[instr.X]; ok {
       field := fr.builder.CreateStructGEP(p, instr.Field, instr.Name())
-      if fr.canAvoidElementLoad(*instr.Referrers()) {
+      if fr.canAvoidElementLoad(instr) {
         fr.ptr[instr] = field
       } else {
         fr.env[instr] = newValue(fr.builder.CreateLoad(field, ""), fieldtyp)
       }
     } else {
       value := fr.llvmvalue(instr.X)
       field := fr.builder.CreateExtractValue(value, instr.Field, instr.Name())
       fr.env[instr] = newValue(field, fieldtyp)
case *ssa.Index:
     i0 := fr.builder.CreateICmp(llvm.IntSLT, index, zero, "")
     li := fr.builder.CreateICmp(llvm.IntSLE, arraylen, index, "")
 
     cond := fr.builder.CreateOr(i0, li, "")
 
     fr.condBrRuntimeError(cond, gccgoRuntimeErrorARRAY_INDEX_OUT_OF_BOUNDS)
 
     addr := fr.builder.CreateGEP(arrayptr, []llvm.Value{zero, index}, "")
-    if fr.canAvoidElementLoad(*instr.Referrers()) {
+    if fr.canAvoidElementLoad(instr) {
       fr.ptr[instr] = addr
     } else {
       fr.env[instr] = newValue(fr.builder.CreateLoad(addr, ""), instr.Type())
     }
 
   case *ssa.IndexAddr:
     x := fr.llvmvalue(instr.X)
     index := fr.llvmvalue(instr.Index)

llgo/trunk/test/irgen/avoidload.go

+// RUN: llgo -S -emit-llvm -o - %s | FileCheck %s
+
+package foo
+
+type X struct {
+  indices [1]int
+}
+
+// CHECK-NOT: load [200 x i64]
+// CHECK: call void @llvm.memcpy.p0i8.p0i8.i64({{.*}}, i64 160000, i32 1, i1 false)
+var _ = [100][200]int{}[0][0]
+
+// CHECK-NOT: load [1024 x i64]
+// CHECK: call void @llvm.memcpy.p0i8.p0i8.i64({{.*}}, i64 8192, i32 1, i1 false)
+var _ = [1024]int{}[X{}.indices[0]]