Bug 58970 - [i965] brw_fs_copy_propagation.cpp:432:21: error: variable length array of non-POD
Summary: [i965] brw_fs_copy_propagation.cpp:432:21: error: variable length array of no...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium blocker
Assignee: Kenneth Graunke
QA Contact:
URL:
Whiteboard:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2013-01-03 06:45 UTC by Vinson Lee
Modified: 2013-01-12 23:44 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
A workaround to make it compile with Clang (1.99 KB, patch)
2013-01-08 07:44 UTC, Kenneth Graunke
Details | Splinter Review

Description Vinson Lee 2013-01-03 06:45:18 UTC
mesa: 622d96aae499445f12861214354a5b9f63e3a738 (master)

Build i965 with clang.

$ make
[...]
  CXX    brw_fs_copy_propagation.lo
clang: warning: argument unused during compilation: '-fno-builtin-memcmp'
brw_fs_copy_propagation.cpp:41:1: warning: 'acp_entry' defined as a struct here
      but previously declared as a class [-Wmismatched-tags]
struct acp_entry : public exec_node {
^
./brw_fs.h:55:4: note: did you mean struct here?
   class acp_entry;
   ^~~~~
   struct
brw_fs_copy_propagation.cpp:432:21: error: variable length array of non-POD
      element type 'exec_list'
   exec_list out_acp[cfg.num_blocks][ACP_HASH_SIZE];
Comment 1 Vinson Lee 2013-01-03 07:49:06 UTC
6a514494fa4c45e921bd6af7f3187a67c1e8d9d2 is the first bad commit
commit 6a514494fa4c45e921bd6af7f3187a67c1e8d9d2
Author: Eric Anholt <eric@anholt.net>
Date:   Fri Sep 21 16:06:17 2012 +0200

    i965/fs: Improve performance of copy/constant propagation.
    
    Use a simple chaining hash table for the ACP.  This is not really very good,
    because we still do a full walk of the tree per destination write, but it
    still reduces fp-long-alu runtime from 5.3 to 3.9s.
    
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

:040000 040000 a3a95c5b01ef5f860d4df61d2e46f41c2dfed611 3881d6355ee5ba71e0f84184ad718ca4a16934d7 M	src
bisect run success
Comment 2 Ian Romanick 2013-01-03 19:57:50 UTC
I actually think this is a compiler bug.  exec_lists have no virtual methods, so they should be POD.
Comment 3 Kenneth Graunke 2013-01-08 07:44:26 UTC
Created attachment 72666 [details] [review]
A workaround to make it compile with Clang

I agree with Ian---I think it's a Clang bug.

I've attached a patch which works around the issue.  I don't know if we should apply it or try to work with upstream.  Ian, Eric?
Comment 4 Kenneth Graunke 2013-01-12 23:44:06 UTC
Well, Matt was the only one that expressed an opinion, and I think it's worth fixing this, so I've pushed the fix:

commit 2c4ad502ce12259160be6c73ebdd6e73a5d27c6f
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Tue Jan 8 12:46:05 2013 -0800

    i965: Fix build error with clang.
    
    Technically, variable sized arrays are a required feature of C99,
    redacted to be optional in C11, and not actually part of C++ whatsoever.
    
    Gcc allows using them in C++ unless you specify -pedantic, and Clang
    appears to allow them for simple/POD types.
    
    exec_list is arguably POD, since it doesn't have virtual methods, but I
    can see why Clang would be like "meh, it's a C++ struct, say no", seeing as
    it's meant to support C99.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=58970
    Reviewed-by: Matt Turner <mattst88@gmail.com>


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.