This is an archive of the discontinued LLVM Phabricator instance.

SROA produces miscompiled code for bitfield access on big-endian targets
AbandonedPublic

Authored by labrinea on Jun 10 2015, 6:16 AM.

Details

Summary

The attached code is miscompiled when targeting big-endian at all optimisation levels except for -O0. This should print "checksum = 00000008", but actually prints "checksum = 00000000". It is correctly compiled if I change the statement just before the function call to func_13 from l_15.f0 to l_15.f1 (the result of this expression is unused). The only change this causes in the IR is to change the parameter in the call to func_13 from 0x800000180 to 0x800018000.

The problem seems to be in the 'scalar replacement of aggregates' pass. The problem arises because we have a 7-byte type but the alloca is 8 bytes (because it's 4-byte aligned), which causes the aggregate to be split up into two 4-byte slices except one actually ends up being 3 bytes. The pass takes into account endianness, thus adds a shift instruction when inserting an integer to an alloca store:

if(DL.isBigEndian())
  ShAmt = 8 * (DL.getTypeStoreSize(IntTy) - DL.getTypeStoreSize(Ty) - Offset);

In this particular example an integer value of wrong size ({i32}) is passed as parameter to the function that computes the shift amount (‘insertInteger’). This causes a zero shift amount since IntTy = {i64}, Ty = {i32}, and offset = 4 bytes. My patch passes a parameter of Ty = {i24} to ‘insertInteger’.

Diff Detail

Repository
rL LLVM

Event Timeline

labrinea updated this revision to Diff 27436.Jun 10 2015, 6:16 AM
labrinea retitled this revision from to SROA produces miscompiled code for bitfield access on big-endian targets.
labrinea updated this object.
labrinea edited the test plan for this revision. (Show Details)
labrinea added a reviewer: chandlerc.
labrinea set the repository for this revision to rL LLVM.
labrinea added a subscriber: Unknown Object (MLST).

Chandler, this is your code, could you please look at this?

lib/Transforms/Scalar/SROA.cpp
2587

This line is longer than 80 characters, and needs to be reformatted so that you don't exceed that limit.

chandlerc edited edge metadata.Jun 30 2015, 4:33 PM

Sorry I've not gotten to this.

This doesn't seem like the right fix... this is something that I think I got fundamentally wrong in r177055, and I spot by inspection numerous related mini bugs that aren't being triggered.

For example, why doesn't this happen for stores as well? If they're not sharing the same broken logic, why not? I'll try to carve out time to specifically debug this problem, but I fear the fix is going to be a bit more involved than this.

Ok, please keep me informed.

Any update on this?

Chandler, could you please guide me a bit on how to resolve this alternatively?

I have investigated this thoroughly now. I believe I understand all of the interacting pieces.

Sadly, the situation is much, much worse than you might imagine. All of the robust fixes for this expose a terribly worse consequence. Currently, I've not found a robust fix that avoids dramatically regressing our ability to promote to SSA values. =/

There is a fundamental disconnect between SROA/mem2reg and how we are canonicalizing memory accesses. This is quite alarming and a source of great concern to me in addition to the big-endian miscompiles. I'm going to need to talk to several folks and look at abunch of other options to figure out what to actually do here.

After a really, really miserable amount of debugging and work, I've figured out how to fix the multitude of issues that this unearthed. I've landed a patch in r242869 which should address all of the issues I've managed to uncover here. I hope there isn't too much fallout from my fix...

Please let me know if you're still seeing any issues.

The problem is resolved you can now close the issue. Thanks!

labrinea abandoned this revision.Oct 9 2015, 3:42 AM