This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL331979: This patch provides that bitfields are splitted even
in case (authored by spetrovic, committed by ).
Herald added
hfinkel added a comment.
In https://reviews.llvm.org/D39053#1093977, @asb wrote:
> Just wanted to explicitly say that I'm happy the updated patch reflects the
> changes to docs and comments I requested. @hfinkel - are you happy for this
> to land now?
Yes, go ahead.
Some of these benefits
asb added a comment.
Just wanted to explicitly say that I'm happy the updated patch reflects the
changes to docs and comments I requested. @hfinkel - are you happy for this to
land now?
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
apazos added a comment.
Thanks for updating the patch, @spetrovic.
Can we have this committed?
This patch has shown to produce code size improvements for a number of targets
(Mips, X86, ARM, RISC-V).
https://reviews.llvm.org/D39053
___
cfe-commits
spetrovic updated this revision to Diff 143918.
spetrovic added a comment.
Comments addressed
https://reviews.llvm.org/D39053
Files:
include/clang/Driver/Options.td
lib/CodeGen/CGRecordLayoutBuilder.cpp
test/CodeGenCXX/finegrain-bitfield-type.cpp
Index:
mgrang added a comment.
Here is a test case which improves with this patch (for RISCV target). It is
able to detect load/store halfword for size 16 bitfields.
struct st {
int a:1;
int b:8;
int c:11;
int d:12;
int e:16;
int f:16;
int g:16;
} S;
void foo(int
asb added a comment.
Hi @spetrovic - I think Hal Finkel's earlier request to update the comments of
IsBetterAsSingleFieldRun remains unaddressed. It looks like at least the
comment string immediately before `auto IsBetterAsSingleFieldRun` needs to be
updated to reflect the changed behaviour.
mgrang added a comment.
With this patch we get ~1800 bytes improvement on one of our internal
codebases. I also ran spec2000/spec2006 validations (for RISCV) and there were
no regressions. There were also no code size improvements seen in spec.
https://reviews.llvm.org/D39053
mgrang added a comment.
Added @apazos and myself a reviewers since this is something we would be
interested in getting to work.
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
spetrovic added a comment.
ping
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
spetrovic added a comment.
ping
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
petarj added a comment.
Is everyone OK with the patch now?
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
wmi added a comment.
Thanks for the size evaluation. I regarded the change as a natural and limited
extension to the current fine-grain bitfield access mode, so I feel ok with the
change. Hal, what do you think?
https://reviews.llvm.org/D39053
petarj added a comment.
This sounds as a valid improvement. Can we have this code committed?
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
spetrovic added a comment.
ping
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
spetrovic added a comment.
ping
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
spetrovic added a comment.
I tried to compile some important libraries for X86 and MIPS64 within Chromium
with clang/llvm. I have compared results between LLVM trunk, and LLVM trunk
with my patch. There is code size improvement on many libraries, here are some
results:
- X86
libframe
wmi added a comment.
I think it may be hard to fix the problem in backend. It will face the same
issue of store-to-load forwarding if at some places the transformation happens
but at some other places somehow it doesn't.
But I am not sure whether it is acceptable to add more finegrain bitfield
spetrovic added a comment.
ping
https://reviews.llvm.org/D39053
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
spetrovic added a comment.
I was looking if I can reslove this problem in backend. Example:
C code:
typedef struct {
unsigned int f1 : 28;
unsigned int f2 : 4;
unsigned int f3 : 12;
} S5;
void foo(S5 *cmd) {
cmd->f3 = 5;
}
. ll file (without the patch):
hfinkel added a comment.
In https://reviews.llvm.org/D39053#906513, @spetrovic wrote:
> Well, basically I'm just expanding the existing algorithm, why should we
> split fields just in case when current field is integer,
> I'm not resolving specific problem with unaligned loads/stores on MIPS.
spetrovic added a comment.
Well, basically I'm just expanding the existing algorithm, why should we split
fields just in case when current field is integer,
I'm not resolving specific problem with unaligned loads/stores on MIPS.
In this example:
typedef struct {
unsigned int f1 : 28;
hfinkel added a comment.
> With this patch we avoid unaligned loads and stores, at least on MIPS
> architecture.
Can you please explain the nature of the problematic situations?
Also, this patch does not update the comments that describe the splitting
algorithm. That should be improved.
If
spetrovic created this revision.
Herald added subscribers: arichardson, sdardis.
This patch provides that bitfields are splitted even in case when current field
is not legal integer type. For Example,
struct S3 {
unsigned long a1:28;
unsigned long a2:4;
unsigned long a3:12;
};
struct
24 matches
Mail list logo
